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

Adopt controller pattern #73

Merged
merged 2 commits into from
Dec 2, 2020
Merged

Adopt controller pattern #73

merged 2 commits into from
Dec 2, 2020

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Nov 18, 2020

This PR builds on the previous one which created "v0.0.2-unstable"

It attempts to formally adopt the controller naming convention, as described here:

https://restfulapi.net/resource-naming/

A controller resource models a procedural concept. Controller resources are like executable functions, with parameters and return values; inputs and outputs.

This aligns best with the existing APIs, and also avoids some of the complaints regarding REST / HTTP specific interfaces as functional style routes are easier to translate to other transports.

Copy link
Member

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

+1.. This is starting to look a bit like the original ​/credentials​/issueCredential and ​/credentials​/composeAndIssueCredential pattern again ;)

@OR13
Copy link
Contributor Author

OR13 commented Nov 18, 2020

@peacekeeper yes, it is, my objective is to adopt a "strategy for naming / structure" and then use that to defend why we choose the structure we have :)

Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

I'm good with this pattern.

@dlongley
Copy link
Contributor

+1.. This is starting to look a bit like the original ​/credentials​/issueCredential and ​/credentials​/composeAndIssueCredential pattern again ;)

Side note: I think we should use kebab case (lowercase w/hypens) for URLs.

@OR13
Copy link
Contributor Author

OR13 commented Nov 18, 2020

@dlongley @peacekeeper from the same style guide:

https://restfulapi.net/resource-naming/

To make your URIs easy for people to scan and interpret, use the hyphen (-) character to improve the readability of names in long path segments.

http://api.example.com/inventory-management/managed-entities/{id}/install-script-location  //More readable
http://api.example.com/inventory-management/managedEntities/{id}/installScriptLocation  //Less readable

@tplooker
Copy link
Contributor

tplooker commented Nov 25, 2020

Question, why not have API's that are more RESTful e.g HTTP POST /credentials which creates and issues a verifiable credential?

@tplooker
Copy link
Contributor

So for example

HTTP POST /credentials <= Creates and issues a verifiable credential
HTTP POST /credentials/verify <= Requests the verification of a verifiable credential

HTTP POST /presentations <= Creates a verifiable presentation (optional signs the presentation)
HTTP POST /presentations/verify <= Requests the verification of a verifiable presentation

@OR13
Copy link
Contributor Author

OR13 commented Nov 25, 2020

@tplooker "RESTful" is not descriptive enough to avoid an endless design debate... this PR attempts to clarify what "flavor" of rest we are agreeing too upfront, so that PRs can be merged according to conformance to style guide.

I think what you are suggesting is the collection style route naming convention....

EDIT: technically in #73 (comment) you are propoosing both collection:

HTTP POST /credentials
HTTP GET /credentials/:credentialId ?
HTTP PUT /credentials/:credentialId ?
HTTP DELETE /credentials/:credentialId ?

and controller:

HTTP POST /credentials/verify

I am not opposed to mixing these, as long as others agree.

@OR13
Copy link
Contributor Author

OR13 commented Nov 25, 2020

One reason to avoid the collection style naming is that is relies on the route to convey information, making it less obvius how to translate the API to none HTTP interfaces.

@tplooker
Copy link
Contributor

One reason to avoid the collection style naming is that is relies on the route to convey information, making it less obvius how to translate the API to none HTTP interfaces.

Yeap understood, r.e mixing the controller and collections pattern, I think if we do add the holder capacity to the API then we will end up mixing anyway

E.g

HTTP POST /credentials/issue
HTTP POST /credentials/verify
HTTP GET /credentials/:credentialId ?
HTTP PUT /credentials/:credentialId ?
HTTP DELETE /credentials/:credentialId ?

Perhaps this ^ is still cleaner though?

@OR13
Copy link
Contributor Author

OR13 commented Nov 30, 2020

@tplooker I like your example.

@tplooker
Copy link
Contributor

tplooker commented Dec 2, 2020

Given the conversation im happy with the proposed changes.

@OR13 OR13 merged commit 4fae4c5 into master Dec 2, 2020
This was referenced Dec 2, 2020
@OR13 OR13 deleted the feat/adopt-controller-pattern branch March 8, 2021 19:02
msporny pushed a commit that referenced this pull request Jul 25, 2021
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.

None yet

4 participants