[aerogear-dev] New Android Pipeline

Wed Oct 3 09:36:32 EDT 2012

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)

>
>> 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