Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for defining hypermedia affordances #19

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

smizell
Copy link
Contributor

@smizell smizell commented Dec 27, 2016

Resolves #18

Please see the issue above for explanation on this PR. Also, note I bumped the version of the specification to 1.1.0.

cc: @paraskakis @netmilk @kylef

docs/overview.md Outdated
@@ -1,6 +1,6 @@
# API Elements Overview

**Version**: 1.0.0-rc1
**Version**: 1.1.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not at 1.0 yet and I think there's still some other changes we want to make before 1.0 of API Elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great. I'll let you all decide what version you'd like to use for this. Bump the rc number?

along with defining unsafe and idempotent transitions.

While data structures are used for defining semantics around data models,
affrodances are used for defining semantics around hypermedia state transitions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- affrodances
+ affordances

{
"element": "affordance",
"meta": {
"id": "self",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the id is to be unique across the whole API Elements document, I wonder if a better name could be used in this example than self. Self probably would not be unique across multiple resources defined across API Elements document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment below.


The control types used here are based on the types used in [ALPS][]. The meta
property `id` SHOULD be used to define the machine-usable name of the
affordance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linking down to https://github.com/apiaryio/api-elements/pull/19/files#r96659051, I wonder if the id is not the right place to place the relation identifier. A id in API Elements is unique across the document, where as relation identifiers are generally just unique from where they are referenced. For example, there could me multiple occurrences of next, prev, self across multiple resources in an API Elements document. I don't believe using the id allows for this. Perhaps I misunderstand.

Copy link
Contributor Author

@smizell smizell Jan 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think you understand it correctly. Let me explain some of my thinking behind it and also share my concerns and some ideas to resolve it.

For affordances to work, we need a way to define an affordance and a way to reference that definition—similar to the needs of data structures. We need to be able to define affordances at the root level, the resource level, and the request/response level. This is how data structures work currently, and the way in which these elements are referenced is by way of the id and ref element.

Additionally, we have things in place to resolve references, so that all references in a document may be fully resolved while retaining original reference information. The id, ref element, and ref attribute together make this possible to do.

The reason for going with id is that it could use the existing method for definition, reference, and dereference without any new elements, attributes, or meta attributes. It also does not add any new rules to the document as affordances could fit right in easily.

However, I share your concerns. This change risks polluting the namespace while leaving room for collisions, as you point out. As I think through this, the only way to reduce this is to introduce a way to namespace or scope a resource—or really any element—for the sake of referencing and definition. This is because even if we don't use the id and introduce something else, it too will need to be scoped in some way.

One options would be to make the id at the root apply globally to the document, but the id in a resource apply only to that resource. We could either change the spec to allows do this, or provide a way for a resource to have its own local scope. The downside here is that it would have to be done at the Refract level here or break away from how Refract handles it.

Another option, we can change how the id itself works in being able to be scoped within a given element. An element could then define an id and the namespace/scope in which it is found. This too requires some rethinking in regards to Refract or departing from it in this area.

The last option is what I mentioned, and that is to create a new way to define, reference, and dereference affordances while defining that they are scoped to a resource when found in that context. While this wouldn't require a Refract change, it would require quite a bit more rules and plumbing, while duplicating work that is already there.

I am not sure I have a strong opinion here, but I'll think on it. There are some big tradeoffs here. What are your thoughts?

Copy link
Member

@kylef kylef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smizell sorry I didn't review this sooner, I glanced at it during vacation and forgot about it when I came back to the computer.

I like the idea and I think this is great, I've left a few notes/comments and questions.

@smizell
Copy link
Contributor Author

smizell commented Feb 1, 2017

Per a discussion with @kylef, I need to do the following to get this ready:

  • Fix the spelling mistake
  • Add entry to CHANGELOG.md under "Master"
  • remove the version change

@smizell
Copy link
Contributor Author

smizell commented Feb 2, 2017

@kylef Changes made.

Note, we discussed changing the language from SHOULD to MAY and it looks like we decided on SHOULD. I left it there, but let me know if you'd prefer MAY.

Thanks for the help :)

transition (e.g. HTTP.GET or HTTP.HEAD).
- unsafe - A hypermedia control that triggers an unsafe, non-idempotent
state transition (e.g. HTTP.POST).
- idempotent - A hypermedia control that triggers an unsafe, idempotent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the names here are good. Both GET and DELETE are idempotent. So, just saying idempotent is not enough.

What if we make controlType an array of enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this borrows from what has already been done by Mike Amundsen and others in ALPS. It appears they get around this with overloading the "idempotent" term to mean an "unsafe, idempotent" control. This would mean that you would use safe for GET and idempotent for DELETE. Conversely, idempotent in this scenario would not be applied to GET.

I'm going to reach out to Mike and others about this, because I think you bring up a good point. While what we have solves the problem you mention, it is a little confusing to say "idempotent" must also be "unsafe." It may be better to do an enum and let users decide how they describe it.

But one note on that—it may require that idempotent always has either safe or unsafe with it if this is to support automatically mapping to other protocols. I'm betting that's why they went the direction they did.

I'll post what I find here and we can chat about where to go from there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are conversing through emails, I wouldn't mind being cced on them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pksunkara in my email, I listed two options we have in addition to the ALPS way:

  1. An array of enum (like you said)
  2. Two boolean attributes: one for safety, one for idempotency

Another option is to make up our own terms and categories. I just mention that here because it might be something good to just think about. However I can't come up with anything right now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pksunkara per my discussions on the mailing list, I think it would be best to remove this controlType attribute and think through it in another issue/PR. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think the way they are overriding the keywords unsafe and idempotent is confusing and shouldn't be done. I actually like your proposal of boolean attributes. My only concern regarding that is whether "safe + non-idempotent" exists and/or makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually curious if there is something we could do differently with this to make it better. I think I agree with your concern here and think we should move away from it. I have a few thoughts on that along with yours, and I think it would be better to discuss specifically rather than just here in comments.

Thanks for bringing this up. Good catch on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with separating the discussion about this. Please feel free to create an issue and remove the relevant parts from here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants