-
Notifications
You must be signed in to change notification settings - Fork 15
Resource prototypes #16
base: master
Are you sure you want to change the base?
Conversation
rfcs/common-data.md
Outdated
of such actions can respond with 401. That is why you have to write same code | ||
10 times. If you want to change this code you have to change it in 10 places. | ||
This problem can be cause of misspellings and logical errors. | ||
So ability to add common responses in one place for multiple actions is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that the authentication proposal in RFC2 might be a better solution to this specific problem?
The authentication framework would allow you to annotate resources, resource group and actions that require authentication and then it would be possible to describe authentication error apart of the authentication scheme.
I think ultimately this will give API Blueprint tools better semantic information on why a specific error may occur and it can be presented better in rendered documentation. Tools such as Dredd testing tool can utilities this information to test all authenticated API actions with invalid auth tokens to verify they match the specification, this wouldn't be possible with "Common Data". Documentation can annotate which actions are authenticated to make it clearer to consumers of the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The authentication framework is a great idea, but I think that authentication is only one problem which can be solved using Common Data
. Another example: we use some parter's API for several actions. Sometimes it does not work, so we return externalAPIError
. It is common response but it is not connected with authentication.
Should I give another example in this document?
rfcs/common-data.md
Outdated
```apib | ||
# Group Authorized resources | ||
|
||
# Common Data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment data section inside the group? Should it have two ##
to indicate it is inside the group? Is the above group empty?
What about defining an MSON Data Structure and use it as base for Response? Something like this:
Or maybe letting actions have a base action. |
@pksunkara I've added variant with action prototypes. It allows to disable some prototype if it is required. I think it can be very useful if you want to disable some common responses for some actions group or particular action. |
Let's not use the |
If we use MSON inheritance - we will have to set ancestor to all actions, so we'll have no advantages over current situation. I can write 10 times |
@pksunkara can you please give me a bit more information about how MSON Inheritance syntax can be used in this task? |
@pksunkara what if we can set base action for resource group? # Group Authorized resources (AuthError) and in this case all actions from # Group Authorized resources (-InternalError) and we can combine it: # Group Authorized resources (AuthError, -InternalError) what do you think about such variant? |
@pksunkara @kylef could you please tell me what should I change in this PR? Is it ok or not? |
I don't think this RFC is good to go, since we have the capability to leverage the MSON syntax for making stuff DRY but we are not doing it here. |
I didn't get the time to think it through, but the other PRs on this repo (like Headers, Auth ones) will give a good inspiration for starting point. |
@pksunkara If I'm not mistaken RFC about Auth uses similar syntax: separate section for authentication section ( In our project this feature will help a lot and will allow to reduce size of documentation dramatically. So I would appreciate if you could me more concrete comment about disadvantages of proposed variant. |
31da9b4
to
a8f2f6e
Compare
@pksunkara I think I've understood what you have talking about. Please take a look on new rewritten version of RFC. |
rfcs/resource-prototypes.md
Outdated
```apib | ||
# Resource prototypes | ||
|
||
## Common Resource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not have an APIB wide thing like this. I know, a little bit of extra work if we remove this, but I think it's the better way to use them as parent in resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain you idea a bit more? Do you want to put Resource prototypes
section into another place? Or you think that Common Resource
is a wrong name? Unfortunately I haven't understood your idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the Resource prototypes
completely and use the data structures like how you did with Authorized
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Should we mark this structures with some mark, for example Authorized (Resource Prototype) to differ them from objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Any other issues to be fixed?
@pksunkara I've looked through PR again and I think it would be better to add base type to resource prototypes like this: ## Authorized (Resource Prototype)
+ Response 403 (application/json)
+ Attributes
+ status: forbidden (string, required, fixed) I think so because if we don't have separate section for such special data structures it will be difficult to read documentation (especially big one) and search for resource prototypes using grep or something like this. It also will be more difficult to implement such behaviour. For example, if you want to define array with What do you think? |
I am still not sure on the name called "Resource Prototype", but syntax looks good now. @kylef Can you please review it? |
@kylef do you have any comments on this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the motivation here makes a lot of sense, but I think the rationale selected might be heading in the wrong direction.
One of the long term goals (and roadmap items) of API Blueprint is to support other protocols, and to decouple the protocol layer via abstraction.
The resource and actions would be decoupled from the protocol (HTTP) itself. A lot of the example use cases described in the motivation would be involved in the protocol layer. The current rationale for this RFC would be further mixing the protocol layer into the data structures which I think is against the long term goal of separating and abstracting away the protocol layer.
Eventually the protocol specifics of an API should defined in a separate protocol layer which is not tightly coupled to the API semantics. Right now, there isn't a protocol layer where this information would belong and it is mixed into the API semantics. The protocol layer is pretty low on the roadmap as there are a lot of other key building blocks that should be solved beforehand such as MSON Parameters and Headers and State Machine Description.
I think to move forward we need to think how this would fit into protocol abstraction. I'd suggest taking a look at some of the plans and discussions around the API Blueprint future:
- API Blueprint Future - A blog post covering the roadmap and the way to protocol independence.
- Abstract Protocol Layer issue.
- Resource Blueprint - A concept resource-oriented, protocol-independent language.
- API Blueprint long-term roadmap
rfcs/resource-prototypes.md
Outdated
|
||
## Authorized (Resource Prototype) | ||
|
||
+ Response 403 (application/json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like how we're adding serialisation into data structures. Data structures are already decoupled from the serialisation and this feels like a step backwards. The point of creating data structures is that they would be decoupled from the serialisations.
Data Structures are agnostic to serialisation of media types. With MSON, I can describe the data and defer the decision whether they will be send as JSON, XML or HAL over the wire.
I had some ideas about decoupling the serialisation and wrote then into #17 so we can decouple the serialisation from request/response bodies which could perhaps fit in here. But this would likely be more part of the larger picture of abstracting the protocol layer from API Blueprint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylef imho you are trying to solve another problem. I see two independent problems:
- do not repeat identical responses (problem which I want to solve)
- decouple mson from http protocol (problem which you want to solve)
I think we shouldn't mix these problems. I just want to solve small problem which would help to reduce api description dramatically. Decoupling is a bigger problem which can help to describe different API (not only JSON HTTP API) and it requires much more changes so I think it's a good idea to solve it now. My changes do not affect decoupling at all because it just repeats current response structure in new section. When we find solution for bigger problem it will be easy to make changes in resource section and the same changes in resource prototype section but for now it would be logically to make the same structure in resource and resource prototype sections.
I thinks Data Structures
is not the best place for Resource Prototype
items so I prefer to create another section called Resource Prototypes
and move all resource prototypes there. It will be more logically for users and also easier to implement.
@kylef @pksunkara what do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylef I've created separate section for resource prototypes. Structure of resource prototype differs a lot from mson data structures, so it will be better for user to have resource prototypes in separate section because in this case API will be more readable and clear.
apiaryio/drafter#467