Skip to content

Proposal for an objects endpoint #111

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 5 commits into from
Mar 11, 2025
Merged

Conversation

philipnbbc
Copy link
Contributor

@philipnbbc philipnbbc commented Feb 11, 2025

This PR proposes an objects endpoint that includes a list of Flows that references the target media object.

The proposal also adds a property that holds the ID of the Flow that first referenced (via a Flow Segment) the media object. The proposal contains some questions around making the property required rather than optional (as currently specified) and whether a user should be able to set it.

Jira Issue (if relevant)

Jira URL: https://jira.dev.bbc.co.uk/browse/CLOUDFIT-3550

Related PRs

Where appropriate. Indicate order to be merged.

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 Jira Issue (if relevant)
  • Follow-up stories added to Jira

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.

Sorry, something went wrong.

@philipnbbc philipnbbc requested a review from a team as a code owner February 11, 2025 14:27
@philipnbbc philipnbbc force-pushed the philipn-add-objects-endpoint branch from 1a619c3 to e9b10cd Compare February 11, 2025 14:28
@philipnbbc philipnbbc force-pushed the philipn-add-objects-endpoint branch from 2659ce4 to 472bf4f Compare March 5, 2025 14:06
@j616 j616 self-assigned this Mar 5, 2025
Copy link
Contributor

@j616 j616 left a comment

Choose a reason for hiding this comment

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

A couple of questions in line. I'm also wondering if it's worth considering allowing querying by a list of object IDs. Mainly because one of our key use cases is removing all instances of content. This will likely take the form of obtaining a list of object IDs from a flow, querying this endpoint for all those IDs, then calling DELETE against all of those flows with the object IDs. The requests will add up quickly. That said, I don't currently think we expect this feature to be needed often!

@philipnbbc
Copy link
Contributor Author

A couple of questions in line. I'm also wondering if it's worth considering allowing querying by a list of object IDs. Mainly because one of our key use cases is removing all instances of content. This will likely take the form of obtaining a list of object IDs from a flow, querying this endpoint for all those IDs, then calling DELETE against all of those flows with the object IDs. The requests will add up quickly. That said, I don't currently think we expect this feature to be needed often!

Maybe the endpoint should be renamed to /objects-query? That is better to allow a GET /objects-query?object_id= for a simple single object lookup and POST /objects-query for multiple objects. Adding POST /objects would be a little confusing / unexpected.

@j616
Copy link
Contributor

j616 commented Mar 5, 2025

Maybe the endpoint should be renamed to /objects-query? That is better to allow a GET /objects-query?object_id= for a simple single object lookup and POST /objects-query for multiple objects. Adding POST /objects would be a little confusing / unexpected.

I was thinking a list in the query string. But thinking about it now, I seem to remember that when we recently looked into that for other purposes, we found the options unsatisfactory.

@philipnbbc
Copy link
Contributor Author

Maybe the endpoint should be renamed to /objects-query? That is better to allow a GET /objects-query?object_id= for a simple single object lookup and POST /objects-query for multiple objects. Adding POST /objects would be a little confusing / unexpected.
I was thinking a list in the query string.

I didn't think adding potentially hundreds of object IDs into the query string was ideal and that's why a POST seemed better. A POST is also used to make queries to DynamoDB for example.

@j616
Copy link
Contributor

j616 commented Mar 6, 2025

I didn't think adding potentially hundreds of object IDs into the query string was ideal and that's why a POST seemed better. A POST is also used to make queries to DynamoDB for example.

Fair point. Let's leave it for now and see what use cases actually pop up in the real world. I think if we do anything in this space, we should explore how we can provide some level of consistency with the rest of the API.

@philipnbbc
Copy link
Contributor Author

philipnbbc commented Mar 6, 2025

I've changed my mind following James' comments on the query parameter. The object ID is now a path component of the URL. This matches what the S3 API does for example and also makes it more consistent with the other endpoints.

@philipnbbc philipnbbc force-pushed the philipn-add-objects-endpoint branch from 0c493bf to f56ff7f Compare March 6, 2025 16:12
@philipnbbc philipnbbc requested a review from j616 March 6, 2025 16:27
@philipnbbc philipnbbc force-pushed the philipn-add-objects-endpoint branch from f56ff7f to a998870 Compare March 11, 2025 10:58
@philipnbbc philipnbbc merged commit 042b9db into main Mar 11, 2025
5 checks passed
@philipnbbc philipnbbc deleted the philipn-add-objects-endpoint branch March 11, 2025 11:00
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.

None yet

3 participants