[aerogear-dev] New Android Pipeline

Wed Oct 3 12:38:59 EDT 2012

> 
> 
> ----- Original Message -----
> > On Wed, Oct 3, 2012 at 4:51 PM, Marko Strukelj
> > <mstrukel at redhat.com>
> > wrote:
> > >
> > >
> > >> On Wed, Oct 3, 2012 at 3:00 PM, Marko Strukelj
> > >> <mstrukel at redhat.com>
> > >> wrote:
> > >> >
> > >> >
> > >> >> Hello,
> > >> >>
> > >> >>
> > >> >> On Wed, Oct 3, 2012 at 12:37 AM, Marko Strukelj
> > >> >> <mstrukel at redhat.com>
> > >> >> wrote:
> > >> >> >
> > >> >> > Some comments ... (I haven't looked at this that closely
> > >> >> > before
> > >> >> > so
> > >> >> > there are some things here that relate to much earlier
> > >> >> > commits).
> > >> >> >
> > >> >> >
> > >> >> > - Logging seems to be the only android specific thing in
> > >> >> > the
> > >> >> > library ATM. I'm thinking that maybe we could treat it as a
> > >> >> > generic java client library using slf4j maybe, and provide
> > >> >> > pluggability to wire it up to android.util.Log on Android.
> > >> >> >
> > >> >> > - I think Matthias noticed that the API is nicely aligned
> > >> >> > with
> > >> >> > the
> > >> >> > one for iOS, which is exactly what we want. (I didn't
> > >> >> > compare,
> > >> >> > so
> > >> >> > I can't say :)
> > >> >> >
> > >> >> > - Looking at HttpRestProvider get() method doesn't take an
> > >> >> > id
> > >> >> > -
> > >> >> > it's an operation that retrieves a list of all instances -
> > >> >> > presuming URL points to something like http://host/task,
> > >> >> > and
> > >> >> > not
> > >> >> > to something like http://host/task/100.
> > >> >> > (https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/core/HttpRestProvider.java#L46)
> > >> >> >
> > >> >> > That makes sense in the HTTP meaning of GET.
> > >> >>
> > >> >>
> > >> >> I'd use plural for endpoint URIs (e.g. /tasks and not /task)
> > >> >>
> > >> >> We need get() for retrieving _all_ items and get(id) for
> > >> >> retrieving a
> > >> >> single object (=> /tasks/{id}).
> > >> >> (We also need 'queries', eventually)
> > >> >>
> > >> >> Regarding iOS... I left a TODO on the more high level
> > >> >> 'filter'
> > >> >> read
> > >> >> (not implemented in the rest adapter)
> > >> >> =>
> > >> >> https://github.com/aerogear/aerogear-ios/blob/master/AeroGear-iOS/AeroGear-iOS/AGPipe.h#L55
> > >> >>
> > >> >> The 'filterObject' argument *could* be:
> > >> >>  - an id to look up a single entity
> > >> >>  - some more complex query object (e.g. a range or what not)
> > >> >> ==> this all would translate in to a 'get()' invocation,
> > >> >> where
> > >> >> the
> > >> >> method takes an argument.
> > >> >>
> > >> >> For looking up a single entity, of course I could have added
> > >> >> a
> > >> >> 'read(id)', which invokes an underlying 'http.get(id)';
> > >> >> Perhaps
> > >> >> I
> > >> >> add
> > >> >> that read(id) now :)
> > >> >>
> > >> >> Anyways, back to Android... get() is fine for all, BUT
> > >> >> get(id)
> > >> >> is
> > >> >> also needed.
> > >> >
> > >> > I think the question here becomes from what point of view
> > >> > should
> > >> > we
> > >> > understand terms get(), put(), post() ...
> > >> > These are clearly meant as HTTP spec terms, so at least inside
> > >> > this
> > >> > class we should understand them strictly from that POV. So I
> > >> > think
> > >> > in terms of API a better approach would be to have a URL
> > >> > represent
> > >> > a full URL endpoint (asset type + a potential id for an
> > >> > instance).
> > >> >
> > >> > That way GET, POST, PUT, DELETE retain their well defined
> > >> > meaning.
> > >> >
> > >> > A wrapper API around this can then have a more business
> > >> > interface
> > >> > understanding of it (persistence understanding in our case -
> > >> > with
> > >> > operation names like: create, update, findById, query,
> > >> > delete).
> > >> >
> > >> > Or, we could add another series of methods called:
> > >> >
> > >> > getChild(String id), deleteChild(String id), postChild(String
> > >> > id,
> > >> > String content), putChild(String id, String content).
> > >> >
> > >> > But what do you do for entities that are properties of your
> > >> > child
> > >> > resources i.e. http://hostname/customers/100/orders?
> > >> >
> > >> > I think it would be best to have another HttpRestAdapter with
> > >> > a
> > >> > new
> > >> > URL: http://hostname/customers/100/orders.
> > >> >
> > >> >>
> > >> >> >
> > >> >> > It means, that if we want to get a single instance of a
> > >> >> > resource
> > >> >> > (i.e. Task#100) we have to create a new HttpRestProvider
> > >> >> > with
> > >> >> > a
> > >> >> > new URL.
> > >> >> >
> > >> >> > For delete(), OTOH we can execute it with a specific id,
> > >> >> > and
> > >> >> > the
> > >> >> > same goes for put() to perform an update of a child
> > >> >> > resource.
> > >> >>
> > >> >>
> > >> >> Correct, a put on /tasks does make no sense. A delete on
> > >> >> /tasks
> > >> >> would
> > >> >> (I guess in most cases) mean all items are deleted, which
> > >> >> feels
> > >> >> wrong....
> > >> >>
> > >> > Sure, it's all relative to endpoint URL - to decide if it
> > >> > makes
> > >> > sense or not. But if I understand correctly the resource URL
> > >> > in
> > >> > our design should always be pointing to a resource type url
> > >> > (i.e.
> > >> > http://hostname/customers), and never to
> > >> > http://hostname/customers/100, or
> > >> > http://hostname/customers/100/orders), and that is the reason
> > >> > for
> > >> > current semantics of get(), delete(), post(), put() ?
> > >> >
> > >> >> So IMO these put/delete operations make more sense on URIs,
> > >> >> like
> > >> >> /tasks/{id} (update / remove a single item).
> > >> >>
> > >> >>
> > >> >>
> > >> >> >
> > >> >> > So we have GET, and POST operations performed on a 'parent'
> > >> >> > resource, and DELETE, and PUT operations performed on child
> > >> >> > resources. This asymmetry is confusing to me ...
> > >> >> > non-intuitive.
> > >> >>
> > >> >> Hrm... I disagree, as indicated in my sentences on put/delete
> > >> >> above...
> > >> >>
> > >> >> I like Stefan's image on restful URIs:
> > >> >> http://www.infoq.com/resource/articles/rest-introduction/en/resources/figure2.jpg
> > >> >>
> > >> >
> > >> > +1
> > >> > Actually I'd say this exactly describes the API and usage as I
> > >> > proposed it above :)
> > >> > URL defines a resource, and GET, DELETE, POST, PUT operate on
> > >> > that
> > >> > exact resource URL.
> > >> > In this scheme of things you never operate on a 'child'
> > >> > resource,
> > >> > as the 'child' resource is already expressed through a URL
> > >> > (i.e.
> > >> > http://hostname/customers/100, or
> > >> > http://hostname/customers/100/orders - a collection on a child
> > >> > resource).
> > >>
> > >>
> > >> Not sure I follow your 'you never operate on a child resource'
> > >> ...
> > >> For
> > >> me the 'http://hostname/customers/100' is a child resource (->
> > >> member
> > >> of the 'customers' collection)... So a PUT (for updates) against
> > >> that
> > >> 'http://hostname/customers/100' URI, is IMO doing the work on
> > >> the
> > >> child (member of the collection).... while, yes, the
> > >> 'http://hostname/customers/100/orders' is a collection on a
> > >> child
> > >> resource (=> the 'orders collection' of the customer #100)
> > >>
> > >
> > 
> > 
> > Hello,
> > 
> > > I mean in terms of HttpRestAdapter methods. One instance of
> > > HttpRestAdapter represents one URL which (I assert) ATM always
> > > maps to a resource-type URL - i.e. http://hostname/customers, not
> > > http://hostname/customers/100.
> > >
> > 
> > The Pipe (public) API maps to to the 'customers' pipe, for
> > instance:
> > 
> > Pipeline pipeline = new Pipeline("http://hostname");
> > Pipe<Customer> customersPipe = pipeline.add("customers",
> > Customer[].class);
> > 
> > ATM, internally there is one 'http client' (e.g. HttpRestAdapter),
> > that operates (currently) on the 'http://hostname/customers'
> > endpoint.
> > 
> > So it supports something like this:
> > 
> > // read ONE customer:
> > Customer c = customersPipe.read("100");
> > 
> > // give me ALL
> > List<Customer> allCustomers = customersPipe.read();
> > 
> > which means, currently the ONE 'http client' object (read:
> > HttpRestAdapter object) operates here on those 'two' URLs:
> > - http://hostname/customers (base url)
> > - http://hostname/customers/{id} (for a specific customer entity)
> > 
> 
> Ok, I imagined it to work this way. And registering different types
> is required in order to have a working marshal/unmarshal - a layer
> wrapped around GET/POST/... layer handled by HttpRestAdapter.
> 
> So Pipes is already a more of a business type layer - specifically -
> a remote persistence layer for us.
> 
> > 
> > 
> > > So on HttpRestAdapter("http://hostname/customers") operations
> > > operating on 'child' resources would be get(100), delete(100),
> > > put(100, data). While operations operating on a resource itself
> > > would be get(), and post().
> > >
> > > That's why I propose a slightly different operation naming
> > > scheme:
> > > getChild(100), deleteChild(100), putChild(100, data). Even
> > > postChild(101, data) if that made some sense - id field in data
> > > would then have to be null for this to make sense. You still keep
> > > get() for list of all children, delete() for removal of all
> > > children (if we support a removal of a single child, then why not
> > > removal of all children?), post(data) for adding new child when
> > > and only when server is supposed to assign ID, while put(data)
> > > makes no sense.
> > 
> > Ah, so, internally ... a mapping like this would occur:
> > 
> > 
> > customersPipe.read("100") ==> restAdapter.getChild(givenId);
> > customersPipe.read();   ==> restAdapter.get();
> >
> 
> Yes.
>  
> > But, that still IMO would be done with the one object
> > (this.httpProvider)...
> >
> 
> Yes. So that makes Pipes API for java a wrapper around
> HttpRestAdapter.
> Also from this a gather that Pipes API does not use
> GET/POST/PUT/DELETE naming scheme, but it is closely aligned with it
> nonetheless.

Sorry, my bad here. The way I understand is actually that Pipes API is more of a persistence API - layer 2 API if you wish, not GET/POST/PUT/DELETE (layer 1 type) API.

>  
> > 
> > -Matthias
> > 
> > 
> > >
> > >
> > >>
> > >> >
> > >> >> The above picture 'models' URIs for this UML diagram:
> > >> >> http://www.infoq.com/resource/articles/rest-introduction/en/resources/figure1.jpg
> > >> >>
> > >> >
> > >> > Actually I understand this as two layers where figure1 layer
> > >> > API,
> > >> > uses figure2 layer API.
> > >> > That's the business interface layer that delegates to
> > >> > HttpRestAdapter, the two layer approach. In our case
> > >> > persistence
> > >> > view with operations like findById ...
> > >> >
> > >>
> > >> yes figure1 is the 'business' API, and figure2 shows how these
> > >> 'methods' map to the URIs...
> > >>
> > >>
> > >>
> > >> >
> > >> >> (Taken from this article =>
> > >> >> http://www.infoq.com/articles/rest-introduction)
> > >> >>
> > >> >> However the figure2.jpg shows as well, that there maybe cases
> > >> >> where a
> > >> >> DELETE on /tasks (or '/customers/{id}/orders') and a POST on
> > >> >> /tasks/{id} (or '/orders/{id}') can make sense.
> > >> >>
> > >> >> => We need to cover that too...
> > >> >> If I recall correctly those two 'corner cases' are also not
> > >> >> covered
> > >> >> by
> > >> >> the JavaScript library.
> > >> >>
> > >> >>
> > >> >> > - I wouldn't eat exceptions, and return null even in a
> > >> >> > not-yet-real
> > >> >> > exception handling :)
> > >> >> > (https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/core/HttpRestProvider.java#L86)
> > >> >> >
> > >> >> > Really, either don't catch it, or if you do catch it,
> > >> >> > rethrow
> > >> >> > it
> > >> >> > as
> > >> >> > is, or wrap into another - normally RuntimeException is
> > >> >> > perfectly
> > >> >> > fine, so you don't pollute your API with throws
> > >> >> > declarations.
> > >> >>
> > >> >>
> > >> >> +1 on this comment - but he added a TODO already ;)
> > >> >>
> > >> >
> > >> > That's why I wrote 'even in a not-yet-real exception handling'
> > >> > i.e.
> > >> > don't _ever_ do it like that :)
> > >> >
> > >> >>
> > >> >> >
> > >> >> > - I would use IllegalArgumentException here:
> > >> >> > https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/AdapterFactory.java#L34
> > >> >> >
> > >> >> > It's more appropriate, as it's a standard pattern for this
> > >> >> > kind
> > >> >> > of
> > >> >> > use-case. By convention UnsupportedOperationException is
> > >> >> > used
> > >> >> > for
> > >> >> > empty methods where interface contract is not fully
> > >> >> > supported.
> > >> >> >
> > >> >>
> > >> >> +1
> > >> >>
> > >> >>
> > >> >>
> > >> >> > - We have to do something about this "getId"
> > >> >> > (https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/RestAdapter.java#L76)
> > >> >> >
> > >> >> > One idea is to have an annotation - @Id. But scanning for
> > >> >> > annotations needs to be done at init time or lazily on
> > >> >> > first
> > >> >> > use
> > >> >> > ...
> > >> >> > So maybe we could have an abstract base class with abstract
> > >> >> > method
> > >> >> > getId() that every data object has to extend, and
> > >> >> > implement.
> > >> >> > A
> > >> >> > simpler, and more robust solution actually, as compiler
> > >> >> > will
> > >> >> > enforce it so there is no way for not providing one, as
> > >> >> > could
> > >> >> > happen with forgotten or wrong placement of @Id annotation
> > >> >> > for
> > >> >> > example.
> > >> >> >
> > >> >>
> > >> >> I think the problem here is that not every object has an
> > >> >> 'id',
> > >> >> the
> > >> >> field could be name 'recordId' - In JavaScript this is
> > >> >> configurable.
> > >> >> iOS has a TODO here...
> > >> >>
> > >> >
> > >> > That's interesting. Can you give some examples?
> > >> >
> > >> >
> > >> >>
> > >> >> -Matthias
> > >> >>
> > >> >>
> > >> >>
> > >> >> >
> > >> >> > That's about it for now ... :)
> > >> >> >
> > >> >> > - marko
> > >> >> >
> > >> >> >
> > >> >> > ----- Original Message -----
> > >> >> >>
> > >> >> >>
> > >> >> >> Hi guys
> > >> >> >>
> > >> >> >>
> > >> >> >> I did some changes in the android library based on iOS
> > >> >> >> stuff,
> > >> >> >> it's
> > >> >> >> closer to the pipeline adapter implementation. I would
> > >> >> >> appreciate
> > >> >> >> feedback and review.
> > >> >> >>
> > >> >> >>
> > >> >> >> https://github.com/aerogear/aerogear-android/pull/1
> > >> >> >> https://github.com/aerogear/aerogear-android-todo/pull/1
> > >> >> >>
> > >> >> >>
> > >> >> >> Thanks,
> > >> >> >> Passos
> > >> >> >> _______________________________________________
> > >> >> >> aerogear-dev mailing list
> > >> >> >> aerogear-dev at lists.jboss.org
> > >> >> >> https://lists.jboss.org/mailman/listinfo/aerogear-dev
> > >> >> >>
> > >> >> > _______________________________________________
> > >> >> > aerogear-dev mailing list
> > >> >> > aerogear-dev at lists.jboss.org
> > >> >> > https://lists.jboss.org/mailman/listinfo/aerogear-dev
> > >> >>
> > >> >>
> > >> >>
> > >> >> --
> > >> >> Matthias Wessendorf
> > >> >>
> > >> >> blog: http://matthiaswessendorf.wordpress.com/
> > >> >> sessions: http://www.slideshare.net/mwessendorf
> > >> >> twitter: http://twitter.com/mwessendorf
> > >> >>
> > >> >> _______________________________________________
> > >> >> aerogear-dev mailing list
> > >> >> aerogear-dev at lists.jboss.org
> > >> >> https://lists.jboss.org/mailman/listinfo/aerogear-dev
> > >> >>
> > >> > _______________________________________________
> > >> > aerogear-dev mailing list
> > >> > aerogear-dev at lists.jboss.org
> > >> > https://lists.jboss.org/mailman/listinfo/aerogear-dev
> > >>
> > >>
> > >>
> > >> --
> > >> Matthias Wessendorf
> > >>
> > >> blog: http://matthiaswessendorf.wordpress.com/
> > >> sessions: http://www.slideshare.net/mwessendorf
> > >> twitter: http://twitter.com/mwessendorf
> > >>
> > >> _______________________________________________
> > >> aerogear-dev mailing list
> > >> aerogear-dev at lists.jboss.org
> > >> https://lists.jboss.org/mailman/listinfo/aerogear-dev
> > >>
> > > _______________________________________________
> > > aerogear-dev mailing list
> > > aerogear-dev at lists.jboss.org
> > > https://lists.jboss.org/mailman/listinfo/aerogear-dev
> > 
> > 
> > 
> > --
> > Matthias Wessendorf
> > 
> > blog: http://matthiaswessendorf.wordpress.com/
> > sessions: http://www.slideshare.net/mwessendorf
> > twitter: http://twitter.com/mwessendorf
> > 
> > _______________________________________________
> > aerogear-dev mailing list
> > aerogear-dev at lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/aerogear-dev
> > 
> _______________________________________________
> aerogear-dev mailing list
> aerogear-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/aerogear-dev
> 