freeCodeCamp / freeCodeCamp

freeCodeCamp.org's open-source codebase and curriculum. Learn to code for free.
https://contribute.freecodecamp.org
BSD 3-Clause "New" or "Revised" License
405.95k stars 38.14k forks source link

Exercise Tracker API is not RESTful #34933

Closed ProfessorSalty closed 3 years ago

ProfessorSalty commented 5 years ago

The exercise tracker's API, as described on the GitHub repo, is not RESTful. I'm happy to submit a fix for this myself, but I believe these microservice challenges use some automated tests, so I thought it would be best to track the issue here until we're in a good spot for breaking changes. If we only need to change the project's documentation and the example's endpoints then this will be easy.

https://github.com/freeCodeCamp/boilerplate-project-exercisetracker/

Current API

POST /api/exercise/new-user - "new-user" isn't a resource. Better to stick with "users" as the resource and let the HTTP define new-ness. GET /api/exercise/users POST /api/exercise/add - RESTful APIs should avoid using verbs in the path GET /api/exercise/log - This is awkward as "log" is also a verb. Also, we're really getting a log from the user resource, so why not make that clear in the URL?

All of the endpoints exist under /api/exercise, which implies a strange hierarchy of users and user logs being properties of an exercise, which is not right.

Proposal

GET /api/users - Returns all users POST /api/users - Creates a new user GET /api/users/{user_id}/logs?[from][to][limit] - Returns all exercises entered for the specified user POST /api/users/{user_id}/exercises - Logs a new exercise to the specified user

paulywill commented 3 years ago

@ProfessorSalty, is this opinion based or best practice (i.e. referenced somewhere that RESTful should not use verbs)?

@scissorsneedfoodtoo, I would be more than happy to assist you if moving forward.

scissorsneedfoodtoo commented 3 years ago

Hey @paulywill, thanks for the @ mention. Unfortunately I lost this issue in my notifications, so I appreciate you bringing it to my attention again.

Also, that you for your patience, @ProfessorSalty, and for all of your helpful suggestions.

As far as I can tell, everything mentioned in the original comment is best practices, especially the bit about removing exercise from the URL.

Currently the older API projects all have the "type" of API as part of the URL. The Personal Library uses /api/books, the Timestamp Microservice uses /api/timestamp, and so on.

I believe this was done so that someone could have all these APIs running on a single domain without potential conflicts.

However the newer backend projects don't do this. The Sudoku Solver and American British Translator both use /api/.... These projects do use verbs as part of the resource (/api/translate), but I think it's okay because they're action verbs, and other endpoints wouldn't make much sense.

So I think there are a few things we need to discuss.

First, if we want to update this project and break compatibility with already completed exercise trackers. This probably isn't a huge deal because we've done it in the past, and presumably everyone who completed this project with the current requirements already submitted it.

Then, if we do want to update the project, what the endpoints should be. I think the ones suggested above are fine. Also, what we should do about the API "type" in the URL. Do we want it to be /api/exercise/... for this project? Or should we follow a more conventional API scheme as suggested above?

Finally, what we should do for the other API projects. Do we want to update the new backend projects to include the type of API as part of their endpoints (/api/translator/translate). Or should we update the older projects to follow a more conventional REST API convention (/api/[date] instead of /api/timestamp/[date])?

What do you all think @moT01, @nhcarrigan, and @RandellDawson?

RandellDawson commented 3 years ago

I like the idea of /api/timestamp/[date] because as was mentioned by @scissorsneedfoodtoo, a single domain could house multiple APIs.

ProfessorSalty commented 3 years ago

I believe this was done so that someone could have all these APIs running on a single domain without potential conflicts.

That's a point I hadn't considered. IMHO, it would make more sense to scope the /api endpoints to each project rather than the other way around, eg. exercise/api/..., timestamp/api/.... This would be clearer in intent and even allow students to serve clients in the base URL (excerise/public, for example) or pull the API code into a separate application easily.

is this opinion based or best practice (i.e. referenced somewhere that RESTful should not use verbs)?

I wish I had some text to point to as gospel. Really, this is my opinion based on my understanding of RESTful architecture. I'm certainly fallible - I would even say error prone - but I'm confident in my case here even if I can't point to why.

scissorsneedfoodtoo commented 3 years ago

Thanks for your input @RandellDawson, and again for your explanations, @ProfessorSalty.

It seems that either /api/[project]/... or /[project]/api/... would work, though I do agree that the latter is cleaner, and would make it easier to serve different clients like you mentioned.

With the current /api/[project]/... structure, it seems like the easiest way to host all the projects on a single domain would be to use subdomains like timestamp.[domain].com. But it would get messy pretty fast -- timestamp.[domain].com/api/timestamp/....

And subpaths would also be a bit repetitive: [domain].com/timestamp/api/timestamp/....

At least that's my basic understanding. If that's the case, I'm all in favor of switching to /[project]/api/... for the backend projects.

Whatever we decide, it would be good if all the projects used the same URL scheme for consistency.

Also, I know what you mean -- I did some reading up on this last week and there seem to be several different approaches. But all the resources agree on things you pointed out like using pluralized nouns for endpoints, and avoiding verbs whenever possible.

moT01 commented 3 years ago

Trying to give some thoughts here...

Would these projects ever be on a single domain again? They're all on sub-domain.freecodecamp.rocks. So there should never be a conflict - I would vote for the "more conventional REST API convention (/api/[date])". I think what you're saying though, is that a user could host it all at one domain. Then, I like the timestamp/api/[date] style. I would prefer the first option if possible - do we really need to accommodate the ability for a user to host all their projects on a single domain? It's kind of nice I guess. I looked through the projects, the only ones that seem to have this problem are the exercise tracker, the url shortener, and the timestamp-microservice . I may have missed one, but the rest seem to just be api/endpoint in general. I think the better way to go here is to just adjust those three to follow the original proposal style instead of trying to adjust the rest. Note: a good way to see what endpoints the projects are using is to flip through the projects on /learn and look at the test text.

Do we want to update this project and break compatibility with already completed exercise trackers.

We could probably write tests to accept both the current endpoints or the new endpoints to keep old projects from breaking. Although, that might get a little messy.

I dunno, I'm not convinced anything needs to be changed here. I do think the proposal is better, so I won't stand in the way if someone wants at it - but if it isn't broke?

The tasks would seem to be to:

scissorsneedfoodtoo commented 3 years ago

Thanks @moT01, that's a good rundown of everything so far, and potential next steps.

I agree -- if we do decide to standardize the endpoints, /api/... is probably the best option. It's good to know that it's just those three projects that would need to be adjusted.

And while I don't think we'll move from sub domains to sub paths for these projects, learners could do that if they want to (client at domain.com/project or sub-domain.domain.com, and endpoints at domain.com/project/api/... or sub-domain.domain.com/api/...).

At least I think that's right. Please correct me if I'm wrong.

Like you mentioned, we could decide to stick with what we have now because it works. But later when we start teaching Node and Express interactively, it would be good to show best practices in the optional and required projects.

Whether or not we decide to change the endpoints for the URL Shortener and Timestamp Microservice, I could see at least fixing the Exercise Tracker to be more RESTful, even if we stick with the /api/exercise/... scheme.

ProfessorSalty commented 3 years ago

I dunno, I'm not convinced anything needs to be changed here.

I wouldn't claim that it's the highest priority issue facing freeCodeCamp, but when I write a test project as proof of concept or to learn something new, I use that as a reference for my final work. Relying on these projects in this way for RESTful design would be detrimental. These sorts of inaccuracies can also erode trust in the platform.

scissorsneedfoodtoo commented 3 years ago

Closing this issue as fixed with https://github.com/freeCodeCamp/freeCodeCamp/pull/41786 and https://github.com/freeCodeCamp/demo-projects/pull/53.

A larger discussion of the backend projects and existing/possible endpoints is here: https://github.com/freeCodeCamp/freeCodeCamp/issues/41529