Lichess4545 / heltour

Chess tournament management software for the Lichess4545 league
https://www.lichess4545.com/
MIT License
45 stars 38 forks source link

API: Find / Update Pairing #3

Closed lakinwecker closed 8 years ago

lakinwecker commented 8 years ago

Make an API method for searching for a pairing based on one or two player names, with an optional league/season filter. It should return the complete set of information for each pairing.

Make an API method for updating the gamelink, or result of a pairing.

Currently chesster does the "Is moderator or is a member of the pairing" check so we could effectively allow this to work for any authenticated user.

cyanfish commented 8 years ago

I've started working on these endpoints, with documentation on the wiki.

chris-- commented 8 years ago

The current API feels quite "RPCish" and might not be easily extensible. E.g., what happens if we switch black/white and request: GET /api/find_pairing/black/{username}/white/{username}/ ?

The standard REST semantics would make the API look something like this:

GET /api/pairing/1 // get pairing by id GET /api/pairing // get list of pairings GET /api/pairing?player={username} // get all pairings of {username} GET /api/pairing?white={username}&black={username2} // this can be extended easily GET /api/pairing?white={username}&black={username2}&season={season} // ...

Updating works by using PUT on the ID: PUT /api/pairing/1 // replace pairing 1 by this one

Creation of new pairings: POST /api/pairing // returns the new pairing

Deletion of pairings: DELETE /api/pairing/1

What do you think?

cyanfish commented 8 years ago

We had some discussion about this before (whether to use a RESTful API versus API methods specifically targeted to Chesster's needs) and decided to keep the implementation as simple as possible since we have very limited uses for the API. That said, you're right that these two endpoints line up very well with REST. I'll leave it up to @lakinwecker since he's the one that's going to be writing the code to consume the API.

lakinwecker commented 8 years ago

Thanks for the feedback! You are correct that these are RPC calls. I believe this is exactly what we need. I would also call the suggested URLs and concepts RPC as well. See below for a bit of a side-track into the depths of what makes REST amazing, and a blog post by Roy Fielding on why most people only need RPC and why most APIs that people build these days are actually RPC and why that's OK.

For v1 - I'm not keen on spending time building a generalized API let alone a fully RESTful one. We have limited resources, and those resources would be better spent on making the lives of the moderators easier. A handful of RPC calls are exactly what we need given our resources.

That being said, I do prefer the URL formats you suggestd. Having parameters in the query-string if possible, it's easier to see that the name relates to the value than having them in to the path. But whether that's: GET /api/pairing?white={username}&black={username2} or GET /api/find-pairing?white={username}&black={username2} is a minor detail. What that URI looks like exactly isn't that important. In the end, my task on the other side will be the same. I'll still have to know the format of the URI, construct that URI from my data, submit it using GET/POST, know the format of the response, deconstruct it based on pre-existing knowledge of the format. etc. That's fine and to be expected.

Also, we have no need to delete a pairing from the API. Nor do we need to to create a new pairing from the API. Hence my statement about not needing a generalized API.

Small rant

A REST API must not define fixed resource names or hierarchies (an obvious coupling of client and server). Servers must have the freedom to control their own namespace. Instead, allow servers to instruct clients on how to construct appropriate URIs, such as is done in HTML forms and URI templates, by defining those instructions within media types and link relations. [Failure here implies that clients are assuming a resource structure due to out-of band information, such as a domain-specific standard, which is the data-oriented equivalent to RPC’s functional coupling].

A REST API should spend almost all of its descriptive effort in defining the media type(s) used for representing resources and driving application state, or in defining extended relation names and/or hypertext-enabled mark-up for existing standard media types. Any effort spent describing what methods to use on what URIs of interest should be entirely defined within the scope of the processing rules for a media type (and, in most cases, already defined by existing media types). [Failure here implies that out-of-band information is driving interaction instead of hypertext.]

http://roy.gbiv.com/untangled/2008/rest-apis-must-be-hypertext-driven

Unless we go way out of our way to satisfy these constraints along with the myriad of other constraints that REST requires, then we are building RPC endpoints. That's ok though, most people only need RPC. The full benefits of REST aren't necessary. The primary reason to aim for the full benefits of REST is to make a protocol that's designed to outlive organizations and specific software (think decades and the myriad of browsers and servers that have come and gone). We're making a set of RPC endpoints to do a few specific things and if they run the league for 2 years, that will be an outstanding success. I highly recommend that blog post, it's a good read.

chris-- commented 8 years ago

@lakinwecker You have a valid point with many APIs being labeled as REST while not at all complying to the semantics. The discussion on REST is long, so let me just give my 2¢. I'm not supposing to create a full blown HATEOAS REST API for some simple internal call. That blog post you linked talks about APIs used to connect systems and that is where HATEOAS shines. I suggest Richardsons Maturity Model as it allows to reason about APIs that are following the REST semantics while not being true REST APIs.

Personally, I use REST semantics for even the simplest kind of my internal APIs. The reason mainly being the consistent semantics and the ease of use with external libraries. Adding a query parameter does not break my routes, neither does changing their order.

I never actually used Django but make sure to check out Flask to see how beautiful and easy APIs are defined there. All the query calls that I described above would boil down to a single route and about 5 lines of code. Hook up an SQLite DB + auth and you have your "Pairing Service" ready to ship.

chris-- commented 8 years ago

@cyanfish Couldn't there be more than one pairing for any of the find_pairing calls? The result should then be an array instead of a key/val:

[
    {
        "season_id": 4,
        "white_team": "The Little Forkers",
        "white_team_number": 10
        "black_team": "Knife f5",
        "black_team_number": 5,
        "white": "brianmcdonald",
        "white_rating": 2076,
        "black": "cyanfish",
        "black_rating": 2159,
        "game_link": "http://en.lichess.org/FVDwoKeI",
        "result": "0-1",
        "datetime": "2016-07-21T22:00:00Z"
    }
]
cyanfish commented 8 years ago

@chris There could theoretically be more than one, but it's not a normal use case and AFAIK the client has no meaningful way to handle multiple results. Instead I have the error case "ambiguous".

lakinwecker commented 8 years ago

Agreed. My client doesn't need lists for this RPC method.

lakinwecker commented 8 years ago

@chris: Regarding REST and HATEOAS - you make good points. I haven't looked at what flask does, but I know that django has equivalent REST style libraries. As for the external 3rd party libraries - we don't have a need for those at the moment. Django's auto-admin will suffice and give us plenty of flexibility with the first set of features.

Again, neither cyanfish (I think?) nor I (I'm sure) have the time to look into it and what cyanfish has already done is good enough. If you're offering to contribute said API and then maintain it for us, I'd be happy to review a PR with that in it. Otherwise, making or supporting it is beyond what I'm going to have time to do while we focus on getting an initial version working.

chris-- commented 8 years ago

I agree that the API is perfectly fine for the use case, if you ever need to extend beyond what is already built than I'd happily volunteer to help.

cyanfish commented 8 years ago

Closing this since these endpoints are fully functional.