Resource Filter API ( was: [VOTE] Release Apache Sling Resource Filter version 1.0.0)

[Robert Munteanu <ro...@apache.org>]

Hi Jason,

On Tue, 2018-08-21 at 08:30 -0400, Jason E Bailey wrote:
> Hi Robert,
> 
> I'll need to clarify the documentation. 
> 
> The ResourceFilter is the object that converts a String to a
> Predicate<Resource> , there are cases where you want the
> ResourceFilter Script to evaluate against a provided java object, The
> ResourceFilter object has methods to add objects to the context that
> the parsed script is ran against.
> 
> Because of that, I choose to make the ResourceFilter Object maintain
> state. Since it maintains state, I needed a provider to create a
> unique instance of ResourceFilter. I couldn't just do new
> ResourceFilter() because I need to encapsulate and separate the
> parser references and I eventually will be adding an ability to
> define new functions for the ResourceFilter Script by way of
> additional services.

I took a look again at the exposed API classes and try to explain to
myself what they do. Again, this is subjective and might be incorrect,
so take it with a grain of salt. 

1. ResourceFilter

This looks like it has the job of building Predicate<Resource>
instances.

2. ResourceFilterException

Checked exception, signals parsing errors

3. ResourceFilterStream

Utility class to create a stream from a resource + filter

4. ResourceStream

Class that is able to create a stream of of resources.

Is that correct?

------

My notes are as follows:

a. Do we need custom exceptions? I.e., do we expect consumers to
try/catch when parsing and then do something about the error? If yes,
we keep the exception, otherwise we could resort to something like an
IllegalArgumentException

b. The ResourceFilter seems to conflate building instances (parse())
and accumulating parameters (addParameter). Not a big deal but can be a
bit hard to parse at first ( excuse the pun ).

Typically I'd see these as builder classes, with an API such as

    ResourcePredicates.withParameter(...).parse(....)

c. The ResourceFilterStream is an almost 1:1 wrapper over the
ResourceStream. Do we really need both?

d. Adapting a resource to a ResourceFilter is a bit confusing, as the
ResourceFilter does not have any state based on the resource it adapts
from. This also tied into b. above.

Thanks for your patience with the release and the review :-)

Best,

Robert

Re: Resource Filter API ( was: [VOTE] Release Apache Sling Resource Filter version 1.0.0)

[Robert Munteanu <ro...@apache.org>]

Thanks for iterating on this - from my point of view the API looks
quite good.

Thanks,

Robert

On Thu, 2018-08-23 at 11:37 -0400, Jason E Bailey wrote:
> I appreciate the more in depth feedback, when you've worked on
> something for a while it's hard to see it from a different point of
> view.
> 
> - Jason
> 
> On Thu, Aug 23, 2018, at 5:19 AM, Robert Munteanu wrote:
> > My notes are as follows:
> > 
> > a. Do we need custom exceptions? I.e., do we expect consumers to
> > try/catch when parsing and then do something about the error? If
> > yes,
> > we keep the exception, otherwise we could resort to something like
> > an
> > IllegalArgumentException
> 
> That's a solid concept on when to implement a custom exception. I
> think it streamlines it to make it an IllegalArgumentException
> 
> > b. The ResourceFilter seems to conflate building instances
> > (parse())
> > and accumulating parameters (addParameter). Not a big deal but can
> > be a
> > bit hard to parse at first ( excuse the pun ).
> > 
> > Typically I'd see these as builder classes, with an API such as
> > 
> >     ResourcePredicates.withParameter(...).parse(....)
> 
> I'll address this below.
> 
> > c. The ResourceFilterStream is an almost 1:1 wrapper over the
> > ResourceStream. Do we really need both?
> 
> Yes. I've bounced back and forth enough between combining them and
> separating them that I feel I have a good grasp on this one. The
> ResourceStream has a very specific function of  creating
> Stream<Resource> objects. Ideally it shouldn't even be it's own class
> and at some point these methods will be moved into the Resource
> api.  With that in mind, in the future, I would deprecate this class
> and not methods in a combined ResourceStream/ ResourceFilterStream.
> Additionally the ResourceFilterStream is doing something different
> where it's maintaining state and allows you to define a branch
> selector and child selector. Having this combined makes it cleaner to
> construct code that will find specific resources. Otherwise it starts
> to get unwieldy.   
> 
> > d. Adapting a resource to a ResourceFilter is a bit confusing, as
> > the
> > ResourceFilter does not have any state based on the resource it
> > adapts
> > from. This also tied into b. above.
> 
> Agreed, this is the part that I'm having the most problem with,
> because I'm trying not to expose any of the internal classes. My
> options as far as I'm aware is to use adaptTo or a service. The
> adapter doesn't feel right as you've pointed out, but requires less
> exposure of classes. When using a Service, I have to create a factory
> which is better but still feels a bit odd since it just wraps a new
> ResourceFilterImpl().  I played around with doing an OSGi factory
> implementation so that the Reference would be a unique instance each
> time but that is really dependent on forming the Reference annotation
> correctly, which is something I also feel is awkward.
> 
> I'll tackle the Builder pattern again and see what I come up with.
> 
> 
> 

Re: Resource Filter API ( was: [VOTE] Release Apache Sling Resource Filter version 1.0.0)

[Jason E Bailey <je...@apache.org>]

I appreciate the more in depth feedback, when you've worked on something for a while it's hard to see it from a different point of view.

- Jason

On Thu, Aug 23, 2018, at 5:19 AM, Robert Munteanu wrote:
> My notes are as follows:
> 
> a. Do we need custom exceptions? I.e., do we expect consumers to
> try/catch when parsing and then do something about the error? If yes,
> we keep the exception, otherwise we could resort to something like an
> IllegalArgumentException

That's a solid concept on when to implement a custom exception. I think it streamlines it to make it an IllegalArgumentException

> b. The ResourceFilter seems to conflate building instances (parse())
> and accumulating parameters (addParameter). Not a big deal but can be a
> bit hard to parse at first ( excuse the pun ).
> 
> Typically I'd see these as builder classes, with an API such as
> 
>     ResourcePredicates.withParameter(...).parse(....)

I'll address this below.

> c. The ResourceFilterStream is an almost 1:1 wrapper over the
> ResourceStream. Do we really need both?

Yes. I've bounced back and forth enough between combining them and separating them that I feel I have a good grasp on this one. The ResourceStream has a very specific function of  creating Stream<Resource> objects. Ideally it shouldn't even be it's own class and at some point these methods will be moved into the Resource api.  With that in mind, in the future, I would deprecate this class and not methods in a combined ResourceStream/ ResourceFilterStream. Additionally the ResourceFilterStream is doing something different where it's maintaining state and allows you to define a branch selector and child selector. Having this combined makes it cleaner to construct code that will find specific resources. Otherwise it starts to get unwieldy.   

> d. Adapting a resource to a ResourceFilter is a bit confusing, as the
> ResourceFilter does not have any state based on the resource it adapts
> from. This also tied into b. above.

Agreed, this is the part that I'm having the most problem with, because I'm trying not to expose any of the internal classes. My options as far as I'm aware is to use adaptTo or a service. The adapter doesn't feel right as you've pointed out, but requires less exposure of classes. When using a Service, I have to create a factory which is better but still feels a bit odd since it just wraps a new ResourceFilterImpl().  I played around with doing an OSGi factory implementation so that the Reference would be a unique instance each time but that is really dependent on forming the Reference annotation correctly, which is something I also feel is awkward.

I'll tackle the Builder pattern again and see what I come up with.