[aerogear-dev] New Android Pipeline

Wed Oct 3 11:06:41 EDT 2012

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)

> 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();

But, that still IMO would be done with the one object (this.httpProvider)...

-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