johnbilt - Bulk flow segments #117
Conversation
There was a problem hiding this comment.
I would choose option 4 as POSTing a single segment dict rather than an array could be seen to be an API convenience.
If it is an array then the response needs to handle partial failures, issues with the array and there needs to be a description of what a TAMS should do when there are (partial) failures.
Should a server attempt to register all segments or stop at the first failure? First failure might mean a serial registration process which is not optimal. What if there are duplicates in the array? The specifications says the behaviour is undefined if a segment already exists in the store. Should that remain undefined?
The response should probably return all the segments that failed to be registered. This is probably not an API break as the current API doesn't have the segment in the response. Should it be a minimal segment with an error code and description? Should the segments that were successfully registered also be returned?
|
|
||
| ### Option 3: Rename existing segments end point and add bulk | ||
|
|
||
| Rename the existing `/flows/{flowId}/segments` to be singular `/flows/{flowId}/segment` and then replace the existing `/flows/{flowId}/segments` with the bulk end point |
There was a problem hiding this comment.
I think another downside with this option is it would potentially create an outlier endpoint without equivalent HEAD, GET, and DELETE. It's also at odds with the rest of the APIs resource endpoints which are plural. Although they are largely of the form <plaural_resource_type>/<id>.
docs/adr/0029-bulk-flow-segments.md
Outdated
|
|
||
| * Good - does not break the existing API | ||
| * Good - keeps the number of end points down | ||
| * Bad - adds complexity to the API implementation |
There was a problem hiding this comment.
I suspect the complexity on the spec side isn't that great. The Spec can handle validation/verification of both variants from a schema point of view (see the flow type "one of" list for the body on the flows endpoint). But @philipnbbc has some valid points on the implementation. The alternative would be something like the following pseudo-code:
if type(body) is object:
body = [body]
for segment in body:
validate_segment(segment)
for segment in body:
add_segment(segment)
It's a little more complexity, but nothing too crazy.
|
CI bugs should be sorted in #119 . You may need to re-base for CI to pick up the changes |
Co-authored-by: James Sandford <[email protected]>
I'm going to suggest that in the case of the failure of a single segment then the publish should continue to work through the remaining segments. Unless you're going to make TAMS roll back all the processed segments then you're going to end up with a partial item, so make it as complete as possible. In terms of response - I think it's a 201 for successfully processing all segments (so works for one or multiple) and then for the partial success then this is more tricky. Current the best answer is probably a 200 with a list of failed segments in the body. |
| "object_id": { | ||
| "description": "The object ID of the segment which has failed to register with the TAMS API", | ||
| "type": "string", | ||
| "pattern": "^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$" |
There was a problem hiding this comment.
Remove this pattern as object_id is not limited to a UUID string.
docs/adr/0029-bulk-flow-segments.md
Outdated
| @@ -0,0 +1,75 @@ | |||
| --- | |||
| status: "draft" | |||
There was a problem hiding this comment.
Remember to change this to "accepted".
api/TimeAddressableMediaStore.yaml
Outdated
There was a problem hiding this comment.
Suggestion to change "Create Flow Segment" to "Create Flow Segments" in the /segments POST summary
| "timerange": "[0:0_10:0)", | ||
| "error": { | ||
| "type": "TAMSError", | ||
| "summary": "Flow delete failed because ...", |
There was a problem hiding this comment.
Nit: here and below, change to something like "flow segment creation failed".
| "properties": { | ||
| "failed_segments": { | ||
| "description": "The object ID of the segment which has failed to register with the TAMS API", | ||
| "type": "object", |
There was a problem hiding this comment.
See the actions failure - this should be an array with object items.
There was a problem hiding this comment.
I note that the linter accepted your change but note that in addition to changing the type to array it needs an items with the object type. See for example the pre property
Details
Ability to add flow segments in bulk via the API. This affects the existing segments end point due to semantics and existing functionality. Follows on from ADR 0024
Jira Issue (if relevant)
Jira URL: N/A
Related PRs
Related to ADR 0024, option 5.
Submitter PR Checks
(tick as appropriate)
Reviewer PR Checks
(tick as appropriate)
Info on PRs
The checks above are guidelines. They don't all have to be ticked, but they should all have been considered.