Add swagger json #46

nik-john · 2019-10-15T23:29:37Z

This PR adds the first draft of the swagger.json file that enumerates the Open API specs for the Chapter UI project.

PS: The document is not complete and the reason why I am raising this PR is so no one else is working on this in parallel. Please commit your open API spec changes to this PR

#17

EVERYONE IS WELCOME to commit to this PR with improvements - this is very WIP with lots of rough edges. Because I don't have a lot of time to spend on this, the idea was to just get this off the ground so we can make it better as we go along

nik-john · 2019-10-16T00:50:42Z

For the uninitiated, you can upload the swagger.json file to http://editor.swagger.io/ to see it in action

QuincyLarson · 2019-10-16T06:38:27Z

@nik-john Awesome. I've heard of Swagger but we haven't used it in a project before.

Would anyone who's familiar with Swagger be interested in helping QA this PR?

nik-john · 2019-10-16T07:01:43Z

@QuincyLarson I've not got a ton of experience on it either but if you want to see it in action, you can upload the swagger.json file to http://editor.swagger.io - it's extremely useful for a number of reasons, not least of which is to agree on a API schema. Here are some screenshots of the file in action

koundinya · 2019-10-16T07:03:36Z

are we going to use the swagger doc to create server and client stubs? I see that as a way to unblock front end devs and also write test cases :)

koundinya · 2019-10-16T07:04:30Z

My suggestion would also to have a linter in place early for swagger, I have seen swagger docs get messy very quickly, I like using speccy for this ~ https://github.com/wework/speccy

nik-john · 2019-10-16T07:06:41Z

@koundinya @email2vimalraj Would you like to contribute to this PR and review it? @email2vimalraj - I saw your comment on another PR about the open API spec, therefore tagging you

email2vimalraj · 2019-10-16T07:10:25Z

@nik-john: Super fast man. I would definitely help reviewing it. I’m currently at work and will take a look tonight.

nik-john · 2019-10-16T07:12:47Z

My suggestion would also to have a linter in place early for swagger, I have seen swagger docs get messy very quickly, I like using speccy for this ~ https://github.com/wework/speccy

Speccy seems to be very interesting - I've used JSON Schemas for this a while ago, but this is something we should do once the app is in good shape

email2vimalraj · 2019-10-16T07:14:22Z

OpenApi supports yaml too, the linting won’t be an issue in that case

koundinya · 2019-10-16T07:18:35Z

"but this is something we should do once the app is in good shape" agree! 👍

MirzaChilman · 2019-10-16T07:25:42Z

The pr looks interesting, definitely gonna review it later tonight utc+7

iansltx · 2019-10-16T07:30:50Z

Nitpick here: this is using v2.0 of the spec. OpenAPI Spec 3 has been out for awhile, to the point that swagger-ui supports it just fine for visualizing, and for an API as simple as this one should be we shouldn't end up with too many rough edges. I recently built an OAS3-documented API, albeit via (quite similar to the end output format and manually written) annotations that compiled to JSON rather than building the doc directly by hand, in answer to the inevitable questions y'all should be asking me: "why do you have an opinion on this and why do we care about your opinion" ;)

iansltx · 2019-10-16T07:31:37Z

With the above said, if this spec gets rewritten as OAS3.0, I'd be happy to review it beyond a cursory glance via the Swagger editor.

nik-john · 2019-10-16T07:51:02Z

@iansltx I haven't particularly worked with Swagger/OAS3 recently, but I have converted the JSON to OAS3. Do take a look sometime

iansltx · 2019-10-16T07:52:29Z

Thanks! Calling it a night shortly, but if this hasn't moved out from under my by the time I'm back online again I'll absolutely look through.

allella

Looks good here. Simple find/replace.

hskrasek

@iansltx Asked me to take a look at this due to some time constraints. Everything looks pretty solid, but I do have some suggested changes looking everything over.

I do agree with the comments about adding some OpenAPI linting in place either using Speccy or Stoplight.io's Spectral.

api/swagger.json

hskrasek · 2019-10-16T18:22:01Z

api/swagger.json

+  "externalDocs": {
+    "description": "Find out more about Swagger",
+    "url": "http://swagger.io"
+  },


Are there chapter.io docs that this externalDocs section could refer to instead?

We don't have one as yet - this was just meant as placeholder

vaibhavsingh97 · 2019-10-16T18:37:34Z

@nik-john Aren't we using GraphQL? if yes then do we really need swagger documentation as afik GraphQL comes with documentation. Also, I know it's not a perfect documentation but we can override documentation and can always add something.

nik-john · 2019-10-16T19:01:03Z

@vaibhavsingh97 #2 (comment)

Postgres with Sequalize (no GraphQL for now)

The consensus was that we aren't using GraphQL (unless something changed that I was unaware about)

vaibhavsingh97 · 2019-10-16T19:02:22Z

Thanks, @nik-john for updating me. Will review it

nik-john · 2019-10-16T19:08:03Z

@hskrasek About this

I do agree with the comments about adding some OpenAPI linting in place either using Speccy or Stoplight.io's Spectral.

Would you be open to chipping in with this implementation? I'm afraid I don't have a lot of experience with either of those

vaibhavsingh97 · 2019-10-16T19:17:28Z

Also, the YAML file would be easier to digest and read. Would love to know about your opinion

Would you be open to chipping in with this implementation? I'm afraid I don't have a lot of experience with either of those

I guess, just install the tool locally and try to run the tool across swagger.json and fix the linting issue. I can help you with that if you need any help

PS: Fixing one issue, I found

Co-Authored-By: Franco Correa <[email protected]>

orbitalhop · 2019-10-17T04:38:18Z

are we going to use the swagger doc to create server and client stubs? I see that as a way to unblock front end devs and also write test cases :)

@koundinya, were you thinking of using the codegen CLI tool (provided by swagger) to achieve this? I've found in the past that it's picky about the files exactly matching the swagger schema, so as long as we validate/lint correctly as discussed above then that shouldn't 🤞be a problem.

Here's a link for the curious:
https://swagger.io/tools/swagger-codegen/

allella · 2019-10-17T05:24:52Z

@francocorreasosa deferring to you on merging this one. I thought / requested QL give @nik-john write, but not sure that he did.

bernhard-hofmann · 2019-10-17T05:34:35Z

I'm going to be picky because we're laying foundations here that will influence everything we build on it.

The POST routes should be singular to add singular entities. I.e. /location not /locations. The GET methods follow this so it would seem consistent to me.

I've not fully reviewed it yet, but wanted to mention this before it gets accepted.

iansltx · 2019-10-17T05:44:51Z

Definitely a case of "pick one" for resource naming. Either singular (which makes POSTs and collections feel awkward) or plural (which IMO doesn't, but others may argue that it has other issues). But be consistent either way.

nik-john · 2019-10-17T05:48:22Z

@bernhard-hofmann @iansltx I think it makes sense to be consistent considering that there's no global standard for pluralization. I'll move everything to plural https://stackoverflow.com/questions/6845772/rest-uri-convention-singular-or-plural-name-of-resource-while-creating-it

bernhard-hofmann · 2019-10-17T05:51:37Z

If you're posting one location, why would singular feel awkward?

I think we're on the same page that the resource is either a (singular) location, group, etc, or it could be a collection of locations, groups, etc

I mentioned it because the POST to locations (plural) had a description indicating it was to add a (single) location.

nik-john · 2019-10-17T06:06:04Z

I've changed all the resources to be plural. I think it makes sense to have:

POST https://api.chapter.io/v1/chapters => chapters.push(req.body.chapter);
GET https://api.chapter.io/v1/chapters=> chapters
GET https://api.chapter.io/v1/chapters/:chapterId => chapters.find(c => c.id === req.params.chapterId); etc. considering that it will be impossible to come to a consensus here - it's a matter of picking one and going with it imo. Could you go through the updated doc and see?

iansltx

Bunch of comments here.

Main takeaways:

Not sure we need 400s for invalid ID type. IMO that should end up as a 404; the client shouldn't care about whether an ID was a valid UUID or not.
Flatten out route structures so you don't have to include a ton of IDs just to get to a single sub-resource. The deepest things should be is /resource/{id}/subresource, with that subresource collection containing entities that are available at the top level.
IDs should be UUIDs across the board.
To avoid locking things in, don't define schemas for entities that aren't referenced in endpoints.

Additionally, I'm not seeing requestBody schemas for anything, though I saw a requestBodies path referenced in at least one of the endpoints. Guessing that e.g. user creation will look a bit different than a user PATCH, so we do want to document that difference.

Finally, having a /me endpoint or similar so the currently logged in user can get their info (e.g. chapter membership, upcoming RSVP'd events, etc.) would be useful. Just in case opaque tokens end up getting used for auth, or in case a client wants to treat a JWT as opaque.

package.json

api/swagger.json

nik-john · 2019-10-17T06:54:57Z

@iansltx Thanks for the detailed review. I'll make the fixes tomorrow when I get some time. TIL a lot of stuff 🙇

allella · 2019-10-17T15:58:55Z

@francocorreasosa this still seems to be the pinch point for a lot of other PRs and progress. I know you can help move this along better than me so when @nik-john rejoins I'm hoping you guys can get something merged in.

QuincyLarson · 2019-10-17T21:36:02Z

@nik-john I'm thrilled that you're learning so much.

It sounds like merging this will help unblock a lot of other contributors.

This is better done than perfect, if that makes sense. So I encourage us to merge as soon as we can, then make other tweaks / improvements afterward.

nik-john · 2019-10-17T21:57:43Z

@QuincyLarson Agreed. I'll try and accommodate as many changes as possible as suggested above in the next half hour and then let's plan to merge. Any further changes can be made to master

nik-john · 2019-10-17T22:55:32Z

@iansltx I have addressed a bunch of stuff you brought up - could you please take a second look? I did not get the time to address a few (including the route hierarchy changes) - so I would appreciate it if you could take a stab at it. If you think that can slide for now, I would recommend getting the PR merged asap so everyone else is unblocked

cc: @QuincyLarson

nik-john and others added 2 commits October 15, 2019 16:28

Create swagger.json

9093806

Update user/group/event APIs

7076096

kognise mentioned this pull request Oct 16, 2019

Our Roadmap #47

Closed

7 tasks

Convert Swagger 2.0 to OAS3

c76ad13

nik-john mentioned this pull request Oct 16, 2019

Spiking on the API using Next.js #17

Closed

Rename Group to Chapter

6d67bd2

allella reviewed Oct 16, 2019

View reviewed changes

allella mentioned this pull request Oct 16, 2019

Add terminology to the README #61

Merged

Add sponsor fields

8a00717

hskrasek suggested changes Oct 16, 2019

View reviewed changes

nik-john and others added 4 commits October 16, 2019 20:55

Update api/swagger.json

cca712b

Co-Authored-By: Franco Correa <[email protected]>

Update api/swagger.json

38cd576

Co-Authored-By: Franco Correa <[email protected]>

Update api/swagger.json

3a674e4

Co-Authored-By: Franco Correa <[email protected]>

Update api/swagger.json

2b0e5f8

Co-Authored-By: Franco Correa <[email protected]>

allella mentioned this pull request Oct 17, 2019

Add category to Chapters Table schema #71

Closed

Standardize pluralization

9bc4872

iansltx reviewed Oct 17, 2019

View reviewed changes

Merge branch 'master' into add-swagger-json

c612585

nik-john added 2 commits October 17, 2019 15:51

Update swagger json, sql

64054f0

Update license

0f2794e

Rename group(s) to chapter(s)

90a2768

nik-john requested a review from francocorreasosa October 17, 2019 23:07

francocorreasosa removed their request for review October 17, 2019 23:53

francocorreasosa merged commit b329f73 into freeCodeCamp:master Oct 17, 2019

This was referenced Oct 18, 2019

Spec country code as 2-letter, use i18n-friendly column names for state/zip #72

Merged

Schema image / ER Diagram is out of date #54

Closed

ScottBrenner mentioned this pull request Oct 18, 2019

Discussion: standardize merge workflow #89

Closed

allella mentioned this pull request Oct 21, 2019

Consider using uncle bob's clean architecture for Api #116

Closed

Add swagger json #46

Add swagger json #46

Conversation

nik-john commented Oct 15, 2019 • edited Loading

nik-john commented Oct 16, 2019

QuincyLarson commented Oct 16, 2019

nik-john commented Oct 16, 2019

koundinya commented Oct 16, 2019

koundinya commented Oct 16, 2019

nik-john commented Oct 16, 2019

email2vimalraj commented Oct 16, 2019

nik-john commented Oct 16, 2019

email2vimalraj commented Oct 16, 2019

koundinya commented Oct 16, 2019

MirzaChilman commented Oct 16, 2019

iansltx commented Oct 16, 2019

iansltx commented Oct 16, 2019

nik-john commented Oct 16, 2019

iansltx commented Oct 16, 2019

allella left a comment

Choose a reason for hiding this comment

hskrasek left a comment

Choose a reason for hiding this comment

hskrasek Oct 16, 2019

Choose a reason for hiding this comment

nik-john Oct 16, 2019

Choose a reason for hiding this comment

vaibhavsingh97 commented Oct 16, 2019

nik-john commented Oct 16, 2019 • edited Loading

vaibhavsingh97 commented Oct 16, 2019 • edited Loading

nik-john commented Oct 16, 2019

vaibhavsingh97 commented Oct 16, 2019 • edited Loading

orbitalhop commented Oct 17, 2019

allella commented Oct 17, 2019

bernhard-hofmann commented Oct 17, 2019

iansltx commented Oct 17, 2019

nik-john commented Oct 17, 2019

bernhard-hofmann commented Oct 17, 2019

nik-john commented Oct 17, 2019

iansltx left a comment

Choose a reason for hiding this comment

nik-john commented Oct 17, 2019

allella commented Oct 17, 2019 • edited Loading

QuincyLarson commented Oct 17, 2019

nik-john commented Oct 17, 2019

nik-john commented Oct 17, 2019

nik-john commented Oct 15, 2019 •

edited

Loading

nik-john commented Oct 16, 2019 •

edited

Loading

vaibhavsingh97 commented Oct 16, 2019 •

edited

Loading

vaibhavsingh97 commented Oct 16, 2019 •

edited

Loading

allella commented Oct 17, 2019 •

edited

Loading