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

Resource prototypes #16

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

Resource prototypes #16

wants to merge 7 commits into from

Conversation

goganchic
Copy link

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.
Copy link
Member

@kylef kylef May 4, 2017

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.

Copy link
Author

@goganchic goganchic May 4, 2017

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?

```apib
# Group Authorized resources

# Common Data
Copy link
Member

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?

@pksunkara
Copy link
Contributor

What about defining an MSON Data Structure and use it as base for Response? Something like this:

+ Response (External API Error)

Or maybe letting actions have a base action.

@goganchic
Copy link
Author

goganchic commented May 5, 2017

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

@pksunkara
Copy link
Contributor

Let's not use the Common Data or Prototype kind of sections. Let's try to use the MSON inheritance syntax.

@goganchic
Copy link
Author

goganchic commented May 5, 2017

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 + Response (500) or I can write 10 times +## Some Action (BaseAction) [GET /some_url] it will be no difference. Can you please write an example of you variant? May be I don't understand your idea.

@goganchic
Copy link
Author

@pksunkara can you please give me a bit more information about how MSON Inheritance syntax can be used in this task?

@goganchic
Copy link
Author

@pksunkara what if we can set base action for resource group?

# Group Authorized resources (AuthError)

and in this case all actions from Authorized resources group will have AuthError response. But in this case we have to add new action like Common Data or Prototype to create named prototype AuthError. Is it ok? And if we want to disable some common response for some particular group or actions group we can use minus sign:

# Group Authorized resources (-InternalError)

and we can combine it:

# Group Authorized resources (AuthError, -InternalError)

what do you think about such variant?

@goganchic
Copy link
Author

@pksunkara @kylef could you please tell me what should I change in this PR? Is it ok or not?

@pksunkara
Copy link
Contributor

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.

@pksunkara
Copy link
Contributor

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.

@goganchic
Copy link
Author

@pksunkara If I'm not mistaken RFC about Auth uses similar syntax: separate section for authentication section (## Authentication Schemes like ## Common Data in this PR) and special keyword to show that section should use authentication scheme (+ Authenticated). What is my mistake? I look at the wrong PR? If it's better to use syntax proposed in this comment I can change PR, but now unfortunately I can't understand what's wrong with this PR.

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.

@goganchic goganchic force-pushed the master branch 3 times, most recently from 31da9b4 to a8f2f6e Compare July 13, 2017 22:30
@goganchic
Copy link
Author

@pksunkara I think I've understood what you have talking about. Please take a look on new rewritten version of RFC.

@goganchic goganchic changed the title Common data Resource prototypes Jul 14, 2017
```apib
# Resource prototypes

## Common Resource
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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?

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 it's needed.

Copy link
Author

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?

@goganchic
Copy link
Author

@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 Members now you have to set Array base type and I think it's ok.

What do you think?

@pksunkara
Copy link
Contributor

I am still not sure on the name called "Resource Prototype", but syntax looks good now. @kylef Can you please review it?

@kylef kylef self-requested a review July 18, 2017 10:53
@goganchic
Copy link
Author

@kylef do you have any comments on this PR?

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.

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:


## Authorized (Resource Prototype)

+ Response 403 (application/json)
Copy link
Member

@kylef kylef Jul 26, 2017

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.

Copy link
Author

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:

  1. do not repeat identical responses (problem which I want to solve)
  2. 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?

Copy link
Author

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.

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.

4 participants