Web API for identifiers and identifier sets #230

tsibley · 2021-06-21T20:12:53Z

Refresh of a long-standing WIP from a couple years ago to add an identifier API. It had slightly different but overlapping motivations at the time. I refreshed it on the latest code and added an identifier lookup endpoint which could be used for validation.

Marked as draft because I expect we'll want some further consideration first. We don't have to use this as-is, but I figured it could be a starting point for discussion about what we do want there with much of the plumbing done.

Commit messages contain details.

davereinhart

I tested each of these endpoint successfully. Looks like a useful set! Just a few questions.
(I tried commenting on multiple lines of code below but it's not showing the ones I selected... I need to practice with that feature a little more)

davereinhart · 2021-07-01T00:45:12Z

lib/id3c/api/routes.py

+    return jsonify(set._asdict())
+
+
+@api_v1.route("/warehouse/identifier-sets/<name>", methods = ['PUT'])


Is there a reason you chose PUT instead of POST method here? Also, would it be beneficial to also be able to set the description value for the new record?

Good question. In my experience:

PUT is usually used for idempotent operations acting directly on a specific resource (e.g. one with a unique id, in this case <name>).

POST is usually used for non-idempotent operations acting on a collection resource, to take arbitrary action (e.g. launch a job) or create new records where you don't know the unique id in advance (e.g. auto-numbered records). Responses to POSTs are often 302 Redirects to the newly-created specific resource, which tell the client what the new id is.

There are lots of discussions about this, of course. One reasonable example is https://restfulapi.net/rest-put-vs-post/.

It would indeed probably be beneficial to allow a description to be set. I didn't think it necessary as a first pass (and it wasn't in my old wip), but it wouldn't be hard to add. I'll add it.

Ok, that explanation makes sense. I came across that same page, and the last entry in the comparison table was throwing me off:

Generally, in practice, always use PUT for UPDATE operations. Always use POST for CREATE operations.

But your explanation matches up well with this answer on SO, so thanks for clearing that up for me!

Added support for "description" with additional commits.

davereinhart · 2021-07-01T00:45:46Z

lib/id3c/api/routes.py

+    return "", 201 if new_set else 204
+
+
+@api_v1.route("/warehouse/identifier-sets/<name>/identifiers", methods = ['POST'])


Is it worth considering setting an upper limit to n value here?

Ah, good call. It's always good to consider what limits are appropriate. I'm not sure what reasonable bound would be though… Here's the current summary stats of batch size for all we've minted so far:

min 1 mode 1,040 max 11,900

Stepping back from n, I think what a limit would really seek to prevent is minting reqs that run "forever". We can't guarantee this just through n alone (although very small n might approximate it well enough), so maybe the more appropriate thing would be an explicit time limit. There is some existing limit on request/response duration already at the web server level (both at uWSGI and Apache, I believe), so that may provide enough of a time limit if the corresponding database session is terminated when the request is. But I'm not sure that's the case.

This also means that we may not be able to use this API as-is to mint large batches of identifiers without increasing existing request/response duration limits, possibly to unreasonable levels. An async API would obviate that issue, but then require a minting job queue and polling of job status by the client. (We've talked about this in the past in related convos.)

All of that said, I don't think we currently have a concrete planned use for this minting API endpoint (unlike the other endpoints in this PR). So maybe we just drop it for now and can revive it later after we have a more concrete use to build for?

@seattleflu/core-devs thoughts?

I think it makes sense to drop it or if possible just disable that route until there's a use case for it. I see that the id3c identifier mint command is used when generating barcode labels, but I'm not familiar enough with all of the workflows to know which ones might need this functionality.

Yeah, I agree, reasonable to drop it from this PR. I'd move it to another branch and push that up as a wip, to potentially be picked up by someone else in the future. Would love input from others here too before I do.

Repushed dropping the minting endpoint.

The sole callsite of JsonEncoder, our as_json(), already disallows these, but I'm about to need to pass JsonEncoder directly to Flask and want to avoid them there as well.

We want to serialize datetimes as ISO 8601 strings instead of RFC 822 strings, and we don't want to allow floating point NaNs or ±Infinity. This affects jsonify(), a handy route helper that I'm about to use.

UUIDs show up in our warehouse.identifier table, and I'll soon be returning API responses for those rows.

Provides a validation endpoint for integrations, such as the BBI LIMS, to check specimen ids.

I wrote these endpoints over 2 years ago while integrating identifier minting and barcode label generation, only to end up refactoring the bulk of it out and generating labels via a CLI command instead. With a web interface for minting new identifiers in mind, I kept the code around as a wip branch. I thought of it again as we're thinking about identifier API integration with the BBI LIMS, so I updated the code to account for changes that happened in the interim.

I empirically checked that Flask does the right thing by only reading the request body if there's a Content-Length (and only up to Content-Length bytes).

…hout a body Better allows endpoints to accept requests with an optional body, like PUTs to create/update a resource with optional fields.

Optional, to allow for the pattern of ensuring a set exists without overwriting its description.

davereinhart

Looks good. After this is merged, I'll make some minor changes to this code as part of PR #234 to include the new required use column in the identifier_set table.

tsibley · 2021-07-19T18:45:57Z

Sounds like a plan, thanks!

tsibley · 2021-07-19T19:09:02Z

Deployed.

tsibley requested a review from a team as a code owner June 21, 2021 20:12

tsibley marked this pull request as draft June 21, 2021 20:13

davereinhart reviewed Jul 1, 2021

View reviewed changes

tsibley marked this pull request as ready for review July 2, 2021 18:27

davereinhart mentioned this pull request Jul 2, 2021

Add use column to identifier_set #234

Merged

tsibley added 3 commits July 7, 2021 11:36

json: Disallow NaN and ±Inf when using our JsonEncoder

fa34552

The sole callsite of JsonEncoder, our as_json(), already disallows these, but I'm about to need to pass JsonEncoder directly to Flask and want to avoid them there as well.

api: Replace Flask's default JSON encoder with our own

f0da413

We want to serialize datetimes as ISO 8601 strings instead of RFC 822 strings, and we don't want to allow floating point NaNs or ±Infinity. This affects jsonify(), a handy route helper that I'm about to use.

json: Serialize UUID objects as strings

e778987

UUIDs show up in our warehouse.identifier table, and I'll soon be returning API responses for those rows.

tsibley force-pushed the api/identifiers branch from bb750c4 to 235169a Compare July 7, 2021 18:41

tsibley added 5 commits July 7, 2021 11:58

api: Add endpoint for looking up an identifier by UUID or barcode

6619cac

Provides a validation endpoint for integrations, such as the BBI LIMS, to check specimen ids.

api: Pass the Content-Length check when there's no request content

4e807d8

I empirically checked that Flask does the right thing by only reading the request body if there's a Content-Length (and only up to Content-Length bytes).

api: Extend content_types_accepted() decorator to handle requests wit…

170e147

…hout a body Better allows endpoints to accept requests with an optional body, like PUTs to create/update a resource with optional fields.

api: Support for setting/updating identifier set descriptions

ca2d7f1

Optional, to allow for the pattern of ensuring a set exists without overwriting its description.

tsibley force-pushed the api/identifiers branch from 235169a to ca2d7f1 Compare July 7, 2021 18:58

tsibley requested review from a team and davereinhart July 7, 2021 18:59

davereinhart approved these changes Jul 19, 2021

View reviewed changes

tsibley merged commit 2ef4a5e into master Jul 19, 2021

tsibley deleted the api/identifiers branch July 19, 2021 18:47

tsibley mentioned this pull request Jul 19, 2021

trs/wip/mint-identifiers #239

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web API for identifiers and identifier sets #230

Web API for identifiers and identifier sets #230

tsibley commented Jun 21, 2021 •

edited

Loading

davereinhart left a comment •

edited

Loading

davereinhart Jul 1, 2021

tsibley Jul 1, 2021 •

edited

Loading

davereinhart Jul 1, 2021

tsibley Jul 1, 2021

davereinhart Jul 1, 2021

tsibley Jul 1, 2021 •

edited

Loading

davereinhart Jul 2, 2021

tsibley Jul 2, 2021

tsibley Jul 7, 2021

davereinhart left a comment

tsibley commented Jul 19, 2021

tsibley commented Jul 19, 2021

		return jsonify(set._asdict())


		@api_v1.route("/warehouse/identifier-sets/<name>", methods = ['PUT'])

		return "", 201 if new_set else 204


		@api_v1.route("/warehouse/identifier-sets/<name>/identifiers", methods = ['POST'])

Web API for identifiers and identifier sets #230

Web API for identifiers and identifier sets #230

Conversation

tsibley commented Jun 21, 2021 • edited Loading

davereinhart left a comment • edited Loading

Choose a reason for hiding this comment

davereinhart Jul 1, 2021

Choose a reason for hiding this comment

tsibley Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

davereinhart Jul 1, 2021

Choose a reason for hiding this comment

tsibley Jul 1, 2021

Choose a reason for hiding this comment

davereinhart Jul 1, 2021

Choose a reason for hiding this comment

tsibley Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

davereinhart Jul 2, 2021

Choose a reason for hiding this comment

tsibley Jul 2, 2021

Choose a reason for hiding this comment

tsibley Jul 7, 2021

Choose a reason for hiding this comment

davereinhart left a comment

Choose a reason for hiding this comment

tsibley commented Jul 19, 2021

tsibley commented Jul 19, 2021

tsibley commented Jun 21, 2021 •

edited

Loading

davereinhart left a comment •

edited

Loading

tsibley Jul 1, 2021 •

edited

Loading

tsibley Jul 1, 2021 •

edited

Loading