----- Original Message -----
On Wed, Oct 3, 2012 at 4:51 PM, Marko Strukelj
<mstrukel(a)redhat.com>
wrote:
>
>
>> On Wed, Oct 3, 2012 at 3:00 PM, Marko Strukelj
>> <mstrukel(a)redhat.com>
>> wrote:
>> >
>> >
>> >> Hello,
>> >>
>> >>
>> >> On Wed, Oct 3, 2012 at 12:37 AM, Marko Strukelj
>> >> <mstrukel(a)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/ma...)
>> >> >
>> >> > 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/AeroGea...
>> >>
>> >> 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/fig...
>> >>
>> >
>> > +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.
-Matthias
>
>
>>
>> >
>> >> The above picture 'models' URIs for this UML diagram:
>> >>
http://www.infoq.com/resource/articles/rest-introduction/en/resources/fig...
>> >>
>> >
>> > 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/ma...)
>> >> >
>> >> > 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/ma...
>> >> >
>> >> > 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/ma...)
>> >> >
>> >> > 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(a)lists.jboss.org
>> >> >>
https://lists.jboss.org/mailman/listinfo/aerogear-dev
>> >> >>
>> >> > _______________________________________________
>> >> > aerogear-dev mailing list
>> >> > aerogear-dev(a)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(a)lists.jboss.org
>> >>
https://lists.jboss.org/mailman/listinfo/aerogear-dev
>> >>
>> > _______________________________________________
>> > aerogear-dev mailing list
>> > aerogear-dev(a)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(a)lists.jboss.org
>>
https://lists.jboss.org/mailman/listinfo/aerogear-dev
>>
> _______________________________________________
> aerogear-dev mailing list
> aerogear-dev(a)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(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/aerogear-dev