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

Design APInf REST API for Catalog endpoints #2102

Closed
bajiat opened this issue Feb 10, 2017 · 35 comments
Closed

Design APInf REST API for Catalog endpoints #2102

bajiat opened this issue Feb 10, 2017 · 35 comments
Assignees
Milestone

Comments

@bajiat
Copy link
Contributor

bajiat commented Feb 10, 2017

Description

Questions:

  • Where is the list of existing endpoints?
  • What is the used data model? Is it visible somewhere other than inside code? (should be)
  • How are operation authentications handled?
  • What are the conventions for error handling?

Methods POST:
eg. add something new. Returns created data object on success.

  • Add organization
  • Add API

Do we put PUT methods to other issue? Those are used normally to update existing object. Using POST for that is breaking the convention used in larger API community.

Methods PUT:
eg. update existing object (just information to be updated is required, not the whole metadata set)

  • update organization
  • update API

User story

Definition of done

@bajiat bajiat changed the title Add POST methods to APInf REST API for Catalog endpoints Design APInf REST API for Catalog endpoints Feb 20, 2017
@bajiat
Copy link
Contributor Author

bajiat commented Feb 20, 2017

@brylie Document the datamodel
@kyyberi Create api design and mentor
@matleppa Study how we create the endpoints and participate in design with @kyyberi

@bajiat bajiat assigned brylie, kyyberi and matleppa and unassigned brylie Feb 20, 2017
@kyyberi
Copy link

kyyberi commented Feb 20, 2017

Do we have a folder in repository where we should store design (swagger)?

@kyyberi
Copy link

kyyberi commented Feb 20, 2017

Design will be done in Swaggerhub.com and from there I can sync result to given repository folder.

@kyyberi
Copy link

kyyberi commented Feb 20, 2017

Working environment https://app.swaggerhub.com/api/kyyberi/APInf-REST-API/MVP @matleppa, create an account to swaggerhub and I'll share the API space to you.

@kyyberi
Copy link

kyyberi commented Feb 20, 2017

How do we auth admin users? Basic auth? OAuth2?

@bajiat bajiat added this to the Sprint 37 milestone Feb 20, 2017
@brylie
Copy link
Contributor

brylie commented Feb 20, 2017

It uses a token-based authentication very similar to OAuth2, but perhaps not OAuth2.

@kyyberi
Copy link

kyyberi commented Feb 20, 2017

good

@brylie
Copy link
Contributor

brylie commented Feb 20, 2017

The Meteor Restivus documentation has an example of a user logging in. In a nutshell, the user can POST their credentials to the authentication endpoint, and receives an authToken in return.

@kyyberi
Copy link

kyyberi commented Mar 3, 2017

@kyyberi
Copy link

kyyberi commented Mar 3, 2017

Could you @jawidahmadi do the review of the design?

@brylie
Copy link
Contributor

brylie commented Mar 6, 2017

@jawidahmadi here are a couple of API design guides that can inform our API design:

@brylie
Copy link
Contributor

brylie commented Mar 6, 2017

Specifically, check out the Zalando API Design Principles.

@brylie
Copy link
Contributor

brylie commented Mar 6, 2017

Re: updating specific fields on a document, without updating the whole document: See the Google HTTP Standard Methods: Update documentation:

The standard Update method should support partial resource update, and use HTTP verb PATCH with a FieldMask field named update_mask.

@kyyberi
Copy link

kyyberi commented Mar 6, 2017

I would recommend looking at http://apistylebook.com/design/guidelines/ for guidelines instead of pointing out single one or two. Out of curiosity @brylie , what is the reason to raise Zalando as an example?

@jawidahmadi
Copy link
Contributor

response code for No Content 204
PUT is Idempotent, consider using it to create
POST is safe, consider using it to update

@brylie
Copy link
Contributor

brylie commented Mar 8, 2017

Out of curiosity @brylie , what is the reason to raise Zalando as an example?

The Zalando API Design Guide seems well considered and organized. The documentation is also open source on Github, so we could contribute back or adapt them to our needs.

The spirit of suggesting these design guideline examples was to help @jawidahmadi explore some of the questions he was raising, by finding sources with good experience.

@philippeluickx
Copy link
Contributor

philippeluickx commented Mar 8, 2017

Getting error in my console when clicking show/hide (Swagger viewer):

35d5778….js?meteor_js_resource=true:21
Uncaught TypeError: Cannot read property 'pushState' of undefined()
toggleEndpointListForResource @ 35d5778….js?meteor_js_resource=true:21
callDocs @ 35d5778….js?meteor_js_resource=true:22
dispatch @ 35d5778….js?meteor_js_resource=true:30
m.handle @ 35d5778….js?meteor_js_resource=true:30

Update: also same error when clicking on "POST" or "GET" etc. when trying to close it.

@philippeluickx
Copy link
Contributor

It is not clear to me what "API updates" are (/apis/updates/). Is it just a series of possible parameters? Then it should be in just /apis/?

@philippeluickx
Copy link
Contributor

philippeluickx commented Mar 8, 2017

Try out example for /apis/updates/ gives me
https://apinf.io:3002/kyyberi/APInf-REST-API/1.0.0/apis/updates/?since=7d&id=undefined&limit=undefined&new_apis_only=false&lifecycle=undefined

  • port 3002?
  • kyyberi?

The response body is "no content". We should make sure there is actual content?

@brylie
Copy link
Contributor

brylie commented Mar 8, 2017

@philippeluickx, the try out examples are prone to failure, because this API is in the Design phase. This means, the endpoints do not yet exist.

We are currently seeking feedback on the design itself, rather than testing functionality.

@philippeluickx
Copy link
Contributor

PUT /apis/{id} has a question in the description:

Do we need an indication, which field(s) to update?

Not sure if this is the best approach in a design-phase API?

@philippeluickx
Copy link
Contributor

Do we need better instructions for pagination?

@philippeluickx
Copy link
Contributor

@kyyberi already touched upon it: how about authentication? What actions you need to have authentication, which ones can you do anonymously?

@brylie
Copy link
Contributor

brylie commented Mar 8, 2017

Getting error in my console when clicking show/hide (Swagger viewer):

35d5778….js?meteor_js_resource=true:21
Uncaught TypeError: Cannot read property 'pushState' of undefined(…)
toggleEndpointListForResource @ 35d5778….js?meteor_js_resource=true:21
callDocs @ 35d5778….js?meteor_js_resource=true:22
dispatch @ 35d5778….js?meteor_js_resource=true:30
m.handle @ 35d5778….js?meteor_js_resource=true:30

@philippeluickx that may be related to issue #2211. If it seems to be so, please post the error message there.

@kyyberi
Copy link

kyyberi commented Mar 8, 2017

@philippeluickx Excellent points above. Could you add your API design related feedback and questions to feedback tab (in API profile)? That is where I assume we actually would like to see feedback. I will not ask them to come to Github and add an issue. ....and here something about dog food and so on ;)

@philippeluickx
Copy link
Contributor

@kyyberi Done!
Quick feedback on the feedback process (meta-feedback?):

  • Too complex - 3 fields to fill in for a simple feedback?
  • Modal - not the right UI (discussion can be found online, but consensus is driving towards not using modals for this purpose)
  • Not clear what happens with my feedback. Who gets the feedback, how do I get notified about replies?

@brylie
Copy link
Contributor

brylie commented Mar 9, 2017

@philippeluickx will you please open a Github issue for your Feedback meta-feedback? That way we can improve the Feedback module as part of a design/development task.

@kyyberi
Copy link

kyyberi commented Mar 11, 2017

I heard there was discussion about PUT and POST methods. It is common practice that both can be used to create new object in the system. PUT is valid for creating a resource, but only if the client has the privilege of naming the resource. In our case that is not the case. We identify objects by ID and that is never defined by client. Thus in our API design new items are always created with POST. PUT is used to update existing object.

@kyyberi
Copy link

kyyberi commented Mar 13, 2017

Reason to have /apis/updates is that it gives you just updates (delta) about changes. Putting all in one method (/apis/) makes it bloated and hard to understand/communicate.

@kyyberi
Copy link

kyyberi commented Mar 13, 2017

Did some improvements to show more clearly, which methods require authentication. Closing this issue and pushing it to external developer review.

@philippeluickx
Copy link
Contributor

/apis/updates: So it's kind of a revision history?

@kyyberi
Copy link

kyyberi commented Mar 13, 2017

Yeah

@philippeluickx
Copy link
Contributor

My suggestion would be to rename it to revisions then. It's more concrete.

@kyyberi
Copy link

kyyberi commented Mar 15, 2017

I asked ext developer(s) opinion about the path and they say both is fine, but like original "updates" better.

@kyyberi
Copy link

kyyberi commented Mar 22, 2017

Before implementation we might want to check that harvestor data model is synced and supported by POST/PUT methods https://github.com/apinf/api-harvester

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

No branches or pull requests

6 participants