[aerogear-dev] New Android Pipeline

Tue Oct 2 22:50:05 EDT 2012

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.

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

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

The above picture 'models' URIs for this UML diagram:
http://www.infoq.com/resource/articles/rest-introduction/en/resources/figure1.jpg

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

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

-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