HydraCG / Specifications

Specifications created by the Hydra W3C Community Group
Other
138 stars 26 forks source link

Same IRI must not denote both - the operation and the api resource which supports that operation #121

Closed elf-pavlik closed 7 years ago

elf-pavlik commented 7 years ago

I changed IRIs that denote operations to:

Possibly I should have used instead:

Any resource which supports any of those operations can possibly just reference their IRIs eg.

{
  "@id": "/api/events",
  "operations": [
    "/api#list-events",
    "/api#add-event"
  ]
}

BTW in 1.entry-point.md the entry point /api advertises support for those two operations, possibly a bug?

and don't use https://www.w3.org/TR/json-ld/#embedding

alien-mcl commented 7 years ago

But /api/events#new and /api/events are not the same IRIs? Indeed, in HTTP hash part of the URL must not be sent to the server, but RDF works on IRIs. This was actually on purpose - with a simple trick I can prevent clients with pre-loading capabilities from requesting other resources unnecessarily. Please bare in mind that server could serialize the payload in a different format, i.e. HTML-embedded RDF.

tpluscode commented 7 years ago

I agree with @elf-pavlik on this. I don't think it's a good idea to mix the API's resources with the documentation metadata.

Hash-URI or not, the /api/events/ and /api/events#new would retrieve the same representation from the API. The operations should definitely be part of documentation representation.

alien-mcl commented 7 years ago

Assuming that there is any API documentation. It is not mandatory. Another counter argument is that why a simple link to a resource, which could be considered as a special case of a i.e. GET operation on a resource can have an IRI same as the resource, but an operation with some explicitely defined attributes (i.e. HTTP method) can't?

tpluscode commented 7 years ago

Assuming that there is any API documentation. It is not mandatory.

I still don't think it's a good idea to mix the identifiers. If you wanted to make the documentation URIs dereferencable, you'll end up in a weird place where you would be looking for description of the operation /api/events#new in the representation of the actual collection.

It looks like you consider this a kind of shortcut but I think it's entanglement. Consider this: /api/events is unavailable to unauthorized users. Gives some kind of 401/403 response. Does it mean that it should be impossible to dereference /api/events#new?

Finally there's the Occam's Razor argument. Newcomers to Hydra will not be thrilled to consider stuff like how RDF treats hash URI etc. They'll need simple guidelines:

  1. My actual API resources go under /api/whatever
  2. My Hydra documentation go under /api-documentation or something.

Separate URI space will also make it easier to manage in a server-side implementation.

lanthaler commented 7 years ago

I'd propose to get rid of the identifier for embedded operations altogether. It's completely fine making them blank nodes... and probably also easier to understand for people unfamiliar with Hydra. This is also in line with @alien-mcl's intention:

This was actually on purpose - with a simple trick I can prevent clients with pre-loading capabilities from requesting other resources unnecessarily.

All the other changes in this PR look good to me and fix stuff that has actually been already called out in the original PRs (e.g. method vs. httpMethod). Github makes it pretty hard to keep track of such requests. Do you guys have any suggestions on how to keep a clear record on what changes were requested and which were implemented?

elf-pavlik commented 7 years ago

I've turned operations into blank nodes, which seems the most agreed state as for today.

But /api/events#new and /api/events are not the same IRIs?

I've never said that, I said that /api/events denotes what I consider two different resources: the api resource on which client can perform supported operations

{
  "@id": "/api/events",
  "@type": "hydra:Collection"
}

on of the supported operations on that resource

{
  "@id": "/api/events",
  "@type": "hydra:Operation",
  "method": "GET",
  "returns": "hydra:Collection",
  "operations": [ "/api/events" ]
}

since the same IRI denotes them, it means that one can consider them the same resource, which may result in merging them into

{
  "@id": "/api/events",
  "@type": ["hydra:Operation", "hydra:Collection"],
  "method": "GET",
  "returns": "hydra:Collection",
  "operations": [ "/api/events" ]
}

now operation has a supported operation (itself) which should return an instance of "hydra:Collection", even if your response includes both in separate named graphs (as in @RubenVerborgh's Turtles all the way down) IMO conflating the resource with one of the operations on that resource can cause a lot confusion.

lanthaler commented 7 years ago

Anyone opposed to merging this change? If I don't hear any objection, I'll merge it in the next days

tpluscode commented 7 years ago

:+1: No objections here

I'd propose to get rid of the identifier for embedded operations altogether.

I realize I was referring to operation types and not identifiers. So what I meant was to replace hydra:Operation with an API-specific term like

{
  "@id": "/api/events",
  "operation": [{
    "@type": "/api/documentation#new-event"
  }]
}

That, is similar to the issue tracker demo, right?

elf-pavlik commented 7 years ago

@tpluscode re: "@type": "/api/documentation#new-event" why do you see need for such specific instance of rdfs:Class and making particular operations instances of that specific class? Can you provide examples of few different instances of /api/documentation#new-event (instances which have different values of properties, not just different blank nodes with same values for all the properties)?

alien-mcl commented 7 years ago

As Tomasz pointed, there may be some additional logic bound to such strongly typed operations as in the discussion at https://github.com/HydraCG/Specifications/pull/117#discussion_r114505359 . Still, I agree with @elf-pavlik that this is unneeded for the use case as it is now.

tpluscode commented 7 years ago

Sure, it's not necessary in this UC.

But in my opinion typed operations are pretty much a necessity for a specialized consumer. If you simply want to iterate the operations and display generated forms you won't need them. But bespoke user interface would benefit from the ability to instantly decide what to show based on operations available for a given representation.

Thus it's not about multiple of /api/documentation#new-event but rather quickly determining whether it's there or not. In fact, I wouldn't be surprised if in practice there was only zero or one instance of any specific operation type.

elf-pavlik commented 7 years ago

In the description of this PR I mentioined

BTW in 1.entry-point.md the entry point /api advertises support for those two operations, possibly a bug?

@lanthaler could you please write in a comment snippet of how you think this entry point should look like? I think you should also have github permission to add commit directly to branch of my fork used for this PR https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

lanthaler commented 7 years ago

@lanthaler could you please write in a comment snippet of how you think this entry point should look like?

Done. We should probably find a more elegant way for this though. Left a note

elf-pavlik commented 7 years ago

Thanks @lanthaler I guess we can agree on merging it during the call on Monday, if someone has objections please share them ASAP.