-
-
Notifications
You must be signed in to change notification settings - Fork 554
rest-api: New exercise #1263
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
rest-api: New exercise #1263
Conversation
Cool exercise idea. I've seen people expressing interest in this kind of exercise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmccandless this is impressive. I am going to refrain from giving approval. I am not an expert on REST so do not feel I am a good judge of what might need to be changed. What I see looks very good. I am able to pick up what you are putting down. 👍
I think @petertseng, @Insti or @ErikSchierboom (just a few names off the top of my head) should review this PR and provide feedback as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this exercise! Really practical. I've left some comments.
"comments": [ | ||
"The state of the API database before the request is represented", | ||
"by the input->database object. Your track may determine how this", | ||
"initial state is set", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing period after set
.
}, | ||
"expected": [ | ||
"{", | ||
"\"users\": []", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for readability indent the contents in this string? So that would make it:
"{"
" \"users\": []",
"}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable.
"database": { | ||
"users": [] | ||
}, | ||
"request_body": ["GET /users"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if maybe making this an object would be useful:
"request_body": {
"method": "GET",
"url": "/users"
}
For requests with a payload, we could then add a "payload" field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this, and I suppose it comes down to the question: what is the real focus of this exercise? String parsing? Data management?
If we are going to extract method and URL, I suggest splitting the request
property into get
and post
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the real focus is in providing a REST API over some data. That this will involve some parsing and data management is important, but not the main focus. To me, it would make sense to have the canonical data align more with the HTTP/REST part than the data management part. But that is of course personal preference.
"The input->payload object and the output are represented as arrays", | ||
"of strings to improve readability in this JSON file. However, in ", | ||
"track implementations, it seems to make sense to expect them as a ", | ||
"single string.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ErikSchierboom thoughts on the wording?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should completely circumvent this issue by just using JSON to represent the payload. So instead of:
"payload": [
"{",
" \"user\": \"Adam\"",
"}"
]
why not do:
"payload": {
"user": "Adam"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose if we are moving away from a focus on string parsing that makes sense. It would then be up to tracks to determine how to represent that JSON data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good idea, but there should be a note to implementers somewhere that the payload should be marshalled to a string before being tested. Parsing and validating the payload is a non-trivial part of the whole problem of building a REST API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just my opinion that we should be focusing on the HTTP/REST part, but I do think that makes sense as we already have lots of string parsing exercises but nothing HTTP-related. That's also why I think this is a fantastic new exercise!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Porque no los dos?
You're right that there are a bunch of string parsing exercises and nothing focusing on HTTP traffic, but this exercise will probably be one of the more advanced in most tracks. I think it's legitimate in a late exercise to require several skills to operate in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing and validating the payload...
(Emphasis mine)
On the subject, I have been considering the idea of adding additional tests that would force the user's implementation to validate URLs and payloads as well. Is there an argument to not add such tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any argument against testing validation of URLs and payloads. The argument in favor seems pretty obvious: that's something that real HTTP servers actually have to do reliably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the subject, I have been considering the idea of adding additional tests that would force the user's implementation to validate URLs and payloads as well. Is there an argument to not add such tests?
I would think this would be quite important, so feel free to add them,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go farther and suggest that the expect objects are also represented in the canonical data as raw JSON, with it being the responsibility of the exercise implementer to parse the serialized JSON string which the student should provide and determine equivalence with the canonical expected data.
Agreed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this exercise. The person implementing it for a given track will have to do more work than usual; they need to:
- initialize a database from a fixture
- spin up the student's implementation in its own process
- perform some HTTP requests
- shutdown the student's server
- all hopefully in a reasonable time span
That said, I think that running an HTTP server is a very useful skill for students to have.
@coriolinus From #1254 (comment):
Technically, there is no requirement here for an actual server to be running. A user solution might use one if they wish, but if the actual parsing and processing functions are already isolated, a test suite can simple access those directly. |
I do think it reasonable to include something in the exercise description that indicates that this sort of program is normally used within the context of a web server (and if the current iteration of the description does not adequately do that now, I am happy to revise). |
If anyone is curious what a track implementation might look like for this exercise, I have been developing one for the Python track in tandem with this PR here. |
I never realised this could all be done offline. In my mind, the exercise would be most useful if a user has to actually create a real HTTP server that listened for actual GET/POST requests. |
I agree that the exercise is most useful if the test harness sets up an actual server. Sure, you can fake it by requiring per-endpoint Also, if the test harness is using real HTTP, it decouples the student's implementation from any particular library. In the Python world, Flask and Django apps look pretty different even when they do the same thing, but you'd ideally want either to be useful for passing this exercise. The programmatic API I'd require of students for this exercise would have only two functions:
Everything else would be HTTP requests to the supplied URI; the test harness would handle all the DB setup and sending the start and stop requests to the student's program as part of test setup and teardown. [edit] Yes, this exports the process control work to the student. It's possible to abstract that away from them and keep the process control logic in the test harness, but then it's more difficult to get the actual listening URI from them. [edit 2] Depending on the language, it might make more sense for the |
This was the reasoning for the original implementation that included the HTTP method in the request body, so that separate methods would not be required. Personally, I think requiring an actual server is outside the scope of Exercism, but maybe I'm wrong on that. |
In the interest of collaboration... If we were to require an actual server, there's an easy solution to avoid the |
I agree. This is also how I intend to use this. |
That's pretty reasonable. The reason I had the student's server return its URI was mainly so that it could request an arbitrary free port from the OS by binding to port 0, thereby preventing any accidental collisions with whatever other software happened to be running on the student's machine. Heroku knows more about what free ports are available for its workers than we do. At this point we're well into dogshedding territory though, I think. |
So what's our consensus? If we are going to expect an actual web server, |
I'm not actually certain that We could certainly define a canonical signature for the |
I suppose track implementations can convert the existing format into a valid HTTP request as-is. |
If anything, the expectation that a track implementation should involve an actual server should be mentioned in the |
I just want to throw this in here because I am curious based on this potential new exercise. Normally, the goal of exercism is to learn a new language using exercises, but this could very easily devolve into a framework comparison/contrast from the mentor's perspective. Since this is a simple REST API and REST transcends language specifics, there does remain quite a bit more setup, though. For example, C# would require a different type of project. Ruby would require either sinatra or the massive entity that is rails, which includes a lot more than just rest. Elm wouldn't really be able to do this at all. If the consensus is to just to simulate everything instead of actually using a framework, will 1) that be clear or 2) be really helpful since most people don't roll their own framework logic to handle requests. I want it to be clear that I think being able to have exercises for learning something like REST is very valuable; I just wanted to throw in some possible dissenting opinions along with asking a question because I am curious if someone has an answer or I missed some discussion somewhere else. |
@tuxagon That's exactly why I've been advocating for implementing this using actual HTTP: so that students can use the framework of their choice, or no framework at all, so long as they can serve requests. The alternative is to mock the HTTP portion, which precludes using any framework. It's possible to make it clear; it's harder to make it useful using mocks. |
@coriolinus I agree that mocks would probably not be a great direction to take this. If it is going to be a framework-agnostic implementation, there probably should be something in the README indicating what frameworks are available along with documentation on using them. That's where it feels like this exercise could get blown out of proportion, though. Adding a lot of dependencies and it will be multiple files (is there any other exercise that is multiple files?). In addition, by using a framework, most have generators making it so that a solution could be submitted and the student didn't actually have to really do any coding. That wouldn't even be a fault of the student because they might not even realize. I just feel like there is something not being considered as the larger concept behind this kind of exercise. Once again, I do think it is valuable; I'm just cautious about just throwing this into the mix without some larger discussions. |
@tuxagon I'm going to switch formats to address this point by point.
README notes about available frameworks have to be language specific, so in some sense, it's not our problem. I agree that it's better if track maintainers include a brief note about popular frameworks, but I don't see any reason it needs to be more than a sentence with a few links. The Python track, for example, might just say "You could choose to do this with Django, Flask, or
Depends on the language, and the framework, if any. Really, though, I don't think the issue is whether it's appropriate to require a multiple-file program from a student; it's whether this problem is too big to make a good exercise. The longest I've spent on a single exercism exercise as a student so far was around five hours. I can't remember which one it actually was; it may have been Forth. I know that I could whip up a quick HTTP REST API in less than that time, so I don't really think it's a valid objection.
So? The teachable point here, in my opinion, isn't that the student learn to do this all from scratch; it's that they learn how to use their framework of choice to produce something real. They still have to connect to and modify a database, they still have to route requests, validate input, produce valid output. If they do this by only writing a tiny bit of code, more power to them! That's exactly the result their bosses and clients would want to see if they were being paid to do this. |
I'm thinking larger than this repo, like what to keep in mind as a track maintainer. I want to think through this and ensure this is the right direction.
I want to clarify that I meant system-wise, not complexity-wise. Like when you submit a solution for mentors to look at, is there a way to pull down and view a multi-file solution at the moment? That's what I meant with regard to a larger discussion. If everything already works for multi-files, then this is a non-issue.
Once again, I am thinking from the mentor's perspective. My understanding is that mentors are there to help new people become more idiomatic and learn more about a language. It's a disservice to mentors to have them review plausibly generated code. This could also be a non-issue because it's likely that the mentor would recognize it immediately, but it does mean the mentor has to have that mental note to look at this exercise's code differently. I just wanted to mention it because it's a possible issue. Since exercism's "new" direction really is focused on mentoring, I think considering the mentor in the exercise process is important. |
The exercism CLI and frontend have supported multi-file exercise submission since V1. I would be astonished if they no longer supported this in V2; that would be a big regression. Agree that mentors shouldn't review generated code, but it is on the student to submit only the files that they have written. |
I didn't know that about the multi-file submissions, so thanks for clarifying @coriolinus. At this point then, seeing it in action would actually be helpful. I wanted to understand the mindset behind this type of exercise since it is a larger type of problem than most in scope, not complexity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmccandless Do you want to merge this? I think we've all discussed it about as much as it needs to be discussed.
@coriolinus Merged! |
is it going to be useful to add a case where an /iou wipes the slate clean between the two parties? This JSON has cases where:
one might consider an amount exactly equal to be a natural new case to add |
This has now been suggested by #1439, fantastic. |
Resolves #1254