Skip to content

Clarify expectations of Flow Segment get_url #36

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 7 commits into from
Apr 9, 2024

Conversation

samdbmg
Copy link
Member

@samdbmg samdbmg commented Mar 22, 2024

Details

Adds an ADR proposing some clarifications on how the get_url field on Flow Segments work:

  • Makes get_url mandatory in Flow Segment responses
  • Clarifies expectations around credentials when making segment requests
  • Replaces get_url with a list of get_urls

Pivotal Story (if relevant)

Story URL: https://www.pivotaltracker.com/story/show/187054769

Related PRs

N/A

Submitter PR Checks

(tick as appropriate)

  • PR completes task/fixes bug
  • API version has been incremented if necessary
  • ADR status has been updated, and ADR implementation has been recorded
  • Documentation updated (README, etc.)
  • PR added to Pivotal story (if relevant)
  • Follow-up stories added to Pivotal

Reviewer PR Checks

(tick as appropriate)

  • PR completes task/fixes bug
  • Design makes sense, and fits with our current code base
  • Code is easy to follow
  • PR size is sensible
  • Commit history is sensible and tidy

Info on PRs

The checks above are guidelines. They don't all have to be ticked, but they should all have been considered.

@samdbmg samdbmg force-pushed the sammg-make-flow-segment-get-url-array branch from 53cb68d to e0f9de4 Compare March 22, 2024 12:15
@samdbmg samdbmg marked this pull request as ready for review March 22, 2024 12:21
@samdbmg samdbmg requested a review from a team as a code owner March 22, 2024 12:21
@philipnbbc philipnbbc self-assigned this Mar 22, 2024
Copy link
Contributor

@philipnbbc philipnbbc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comprehension comment about what "identical" is referring to on a first read (happy for you to keep as-is). Otherwise LGTM

@johnbilt
Copy link
Contributor

johnbilt commented Mar 22, 2024

The basic API structure looks good - nice to see labels to identify what the end points are. However at the risk of opening a can of worms what happens about adding or removing URLs after the original segment record has been created?

@samdbmg
Copy link
Member Author

samdbmg commented Mar 27, 2024

What happens about adding or removing URLs after the original segment record has been created?

On a "keep the core API simple" basis, I'd be inclined to say it should be an out-of-band extra that's neither required nor expressly forbidden. From a client's perspective, the list of get_urls they receive is a snapshot, only valid at that point in time, and may change if they make another request, but the semantics ("all URLs are equally valid and interchangeable") still apply.

From a server's perspective, it depends how you've implemented it. Either way you have to provide a put_url and an object_id to the client when they write content. The server might then choose to replicate that content somewhere else (another region?) and it can then generate get_urls covering both regions. Or it might be there's a CDN and/or an on-site cache involved, and it generates a pair of get_urls: one to go via the CDN, and one via the local cache. Or it might be you have A and B resilience ingests, leading to A and B Flows (of the same Source) which are defined to be bit-for-bit identical, and your particular TAMS store implementation responds to a signal that turns that into a separate resilient pair Flow C. Then the store lists URLs for both Flow A objects and Flow B objects in Flow C get_urls (unless one segment is missing, in which case you only get one: that's what the resilience was for!)

The other way to approach it would (potentially) be to allow registering additional duplicate Flow Segments (producing additional URLs), but the trade-off is it makes it easier to break immutability, because a client could come along and register a duplicate segment that differs from the original, and it would be difficult to enforce against that! IMO, we should leave it as is until the need arises to relax that restriction

@samdbmg samdbmg force-pushed the sammg-make-flow-segment-get-url-array branch from 9082f1c to 6c0dfda Compare March 27, 2024 09:32
samdbmg and others added 7 commits April 9, 2024 10:05
Implements part of ADR0015 to make the `get_url` and `put_url` fields in
Flow Segments mandatory.

sem-ver: feature
Updates the description for `get_url` and `put_url` that clients should
include credentials for same-origin requests.
Implements Option 2 of ADR0015.

sem-ver: feature
Renames the Flow Segment `get_url` property to be `get_urls` and be a
list of URLs.
Implements Option 3 of ADR0015.

sem-ver: api-break
@samdbmg samdbmg force-pushed the sammg-make-flow-segment-get-url-array branch from 6c0dfda to dd2d1b0 Compare April 9, 2024 09:07
@samdbmg samdbmg merged commit 7f02670 into main Apr 9, 2024
3 checks passed
@samdbmg samdbmg deleted the sammg-make-flow-segment-get-url-array branch April 9, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants