Skip to content

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

Merged
merged 11 commits into from
Jul 26, 2018
Merged

Conversation

cmccandless
Copy link
Contributor

Resolves #1254

@guygastineau
Copy link
Contributor

Cool exercise idea. I've seen people expressing interest in this kind of exercise.
I'm glad someone is finally getting to it. Good luck!

Copy link
Member

@rpottsoh rpottsoh left a 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.

Copy link
Member

@ErikSchierboom ErikSchierboom left a 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",
Copy link
Member

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\": []",
Copy link
Member

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\": []",
"}"

Copy link
Contributor Author

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"]
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.",
Copy link
Contributor Author

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?

Copy link
Member

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"
}

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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!

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

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,

Copy link
Member

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!

Copy link
Member

@coriolinus coriolinus left a 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.

@cmccandless
Copy link
Contributor Author

@coriolinus From #1254 (comment):

I thought it would be easiest to uncouple the API handling code from any sort of server.

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.

@cmccandless
Copy link
Contributor Author

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).

@cmccandless
Copy link
Contributor Author

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.

@ErikSchierboom
Copy link
Member

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 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.

@coriolinus
Copy link
Member

coriolinus commented Jul 19, 2018

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 head, get, post, put, patch, and delete methods, but that approach actually takes more implementation work than implementing a real HTTP server using a normal library.

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:

  • start(db_path) -> (uri, process_handle): creates a new process and runs the student's server within that process. Connects the student's server to the provided database connection string. Returns the URI at which the server is serving requests and a process handle which is opaque to the test framework. This function should return only after the student's server is prepared to serve requests.
  • kill(process_handle): This function must shut down the student's process.

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 kill function to be a method on process_handle. In Rust, I'd probably require a std::process::Child from the start function, which comes with a kill method built-in. That means that the student actually needs to write only the start function. However, this part of the spec is extremely language-specific.

@cmccandless
Copy link
Contributor Author

Sure, you can fake it by requiring per-endpoint head, get, post, put, patch, and delete methods

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.

@cmccandless
Copy link
Contributor Author

In the interest of collaboration...

If we were to require an actual server, there's an easy solution to avoid the start method having to return a server URL. The address will always be 127.0.0.1, and the port can be passed as an argument to the start method. This functionality is a reasonable requirement, as hosting services like Heroku require web apps to use a port number from the environment.

@ErikSchierboom
Copy link
Member

If we were to require an actual server, there's an easy solution to avoid the start method having to return a server URL. The address will always be 127.0.0.1, and the port can be passed as an argument to the start method. This functionality is a reasonable requirement, as hosting services like Heroku require web apps to use a port number from the environment.

I agree. This is also how I intend to use this.

@coriolinus
Copy link
Member

The address will always be 127.0.0.1, and the port can be passed as an argument to the start method.

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.

@cmccandless
Copy link
Contributor Author

So what's our consensus? If we are going to expect an actual web server, canonical-data.json will need another rework.

@coriolinus
Copy link
Member

I'm not actually certain that canonical-data.json needs to change. As things stand right now, individual tracks are free to implement this exercise by mocking HTTP transactions offline or by requiring an actual server. I'm kind of looking forward to implementing it for the Rust track, where I will take the latter choice.

We could certainly define a canonical signature for the start function and add some tests which imply that signature, but I'm not sure that would add to the exercise. I'm in favor of merging as-is.

@cmccandless
Copy link
Contributor Author

I suppose track implementations can convert the existing format into a valid HTTP request as-is.

@cmccandless
Copy link
Contributor Author

If anything, the expectation that a track implementation should involve an actual server should be mentioned in the comments section.

@tuxagon
Copy link

tuxagon commented Jul 20, 2018

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.
(Note: I know there are others, but I mentioned the big ones for simplicity here)

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.

@coriolinus
Copy link
Member

@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.

@tuxagon
Copy link

tuxagon commented Jul 20, 2018

@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.

@coriolinus
Copy link
Member

@tuxagon I'm going to switch formats to address this point by point.

there probably should be something in the README indicating what frameworks are available along with documentation on using them.

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 http.server"; Rust might say "You might do this with Rocket or hyper". Building a real http server will always be one of the more complicated tasks, so I think it's fine to ask the student to do their own library use research.

Adding a lot of dependencies and it will be multiple files

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.

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.

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.

@tuxagon
Copy link

tuxagon commented Jul 21, 2018

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 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.

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.

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.

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.

@coriolinus
Copy link
Member

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.

@tuxagon
Copy link

tuxagon commented Jul 22, 2018

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.

Copy link
Member

@coriolinus coriolinus left a 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.

@cmccandless cmccandless merged commit 4049505 into exercism:master Jul 26, 2018
@cmccandless
Copy link
Contributor Author

@coriolinus Merged!

@cmccandless cmccandless deleted the new-exercise-rest-api branch July 26, 2018 13:22
@petertseng
Copy link
Member

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:

  • lender owes borrower an amount greater than new loan
  • lender owes borrower an amount less than new loan

one might consider an amount exactly equal to be a natural new case to add

@petertseng
Copy link
Member

is it going to be useful to add a case where an /iou wipes the slate clean between the two parties?

This has now been suggested by #1439, fantastic.

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

Successfully merging this pull request may close these issues.

7 participants