Skip to content

Conversation

@cfm
Copy link
Member

@cfm cfm commented Oct 31, 2025

Closes #7639 (see context there) by letting the client specify an API minor version for the response shape it expects. Currently we have four minor versions:

# 0. Initial implementation
# 1. `Index` and `BatchResponse` include `journalists`
# 2. `Reply` and `Submission` objects include `interaction_count`
# 3. `BatchRequest` accepts `events` to process, with results returned in
# `BatchResponse.events`

Test plan

  • Tests adequately exercise Prefer: securedrop=x versions defined so far.
  • The SecureDrop journalist app at develop works as expected without sending a Prefer header.

Checklist

This change accounts for:

  • any required additional documentation

I will add to #7713 (or a follow-up) once this this is merged.

  • any necessary AppArmor changes (added or removed application files)
  • any impact on new SecureDrop installs and upgrades
  • our dependency update policy

@cfm cfm added this to SecureDrop Oct 31, 2025
@cfm cfm moved this to In Progress in SecureDrop Oct 31, 2025
@cfm cfm requested a review from Copilot October 31, 2025 18:55
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements API versioning support for the SecureDrop API v2, allowing clients to specify a preferred minor version via the Prefer header. The changes conditionally include or exclude certain fields based on the requested API minor version.

  • Added minor parameter to to_api_v2() methods across models to support versioning
  • Implemented version-based conditional field inclusion (e.g., interaction_count only in v2.2+)
  • Added logic to filter top-level response keys based on API minor version

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
securedrop/models.py Updated to_api_v2() methods to accept minor parameter and conditionally include interaction_count field for API v2.2+
securedrop/journalist_app/api2/init.py Added API minor version handling via Prefer header, conditional event processing for v2.3+, and response filtering based on client version
Comments suppressed due to low confidence (1)

securedrop/journalist_app/api2/init.py:142

  • Missing minor parameter in source.to_api_v2() call. This is inconsistent with other calls in the same function (lines 134, 149, 158, 165) and means sources requested directly won't respect the client's API version preference. Change to source.to_api_v2(minor).
            response.sources[source.uuid] = source.to_api_v2()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cfm cfm force-pushed the 7639-apiv2-minor-versioning branch from 02fba8b to d3fe688 Compare October 31, 2025 18:58
@cfm cfm requested a review from legoktm October 31, 2025 19:07
@cfm cfm self-assigned this Oct 31, 2025
@cfm cfm force-pushed the 7639-apiv2-minor-versioning branch 2 times, most recently from 671af27 to e670850 Compare October 31, 2025 19:37
@cfm cfm changed the title feat(/data): Prefer: securedrop=x limits response shape to minor version 2.x feat(/data): Prefer: securedrop=x limits response shape to minor version 2.x Nov 1, 2025

# Process events in snowflake order.
for event in sorted(events, key=lambda e: int(e.id)):
result = handler.process(event)
Copy link
Member Author

Choose a reason for hiding this comment

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

Event handlers will need to be passed minor so that they can version-check consistently:

-        current_version = json_version(source.to_api_v2())
+        current_version = json_version(source.to_api_v2(minor))

Copy link
Member

Choose a reason for hiding this comment

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

maybe we have it as a member variable on handler and turn the event handlers into normal methods instead of static ones?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried it out, and I realized that there's a separation I'd like to preserve, in order to keep the actual handler methods as close to pure functions on (event received, current server state) as possible.

def __init__(self, session: Session, redis: Redis) -> None:
"""
Configure the `EventHandler`. Attributes set here are for internal use
by the `EventHandler`; handler methods are static and do not have access
to them, which means they cannot influence the processing of a given
event.
"""

@cfm cfm moved this from In Progress to Ready For Review in SecureDrop Nov 6, 2025
Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

Overall I like the direction; the conditionals are quite simple right now, if it does get much more complicated we might want to have some explicit serialization class.


# Process events in snowflake order.
for event in sorted(events, key=lambda e: int(e.id)):
result = handler.process(event)
Copy link
Member

Choose a reason for hiding this comment

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

maybe we have it as a member variable on handler and turn the event handlers into normal methods instead of static ones?

@cfm cfm moved this from Ready For Review to In Progress in SecureDrop Nov 12, 2025
@cfm cfm force-pushed the 7639-apiv2-minor-versioning branch 2 times, most recently from 44445c7 to 218f01b Compare November 13, 2025 19:57
@cfm cfm requested a review from Copilot November 13, 2025 19:58
Copilot finished reviewing on behalf of cfm November 13, 2025 20:02
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cfm cfm force-pushed the 7639-apiv2-minor-versioning branch from cfcfdb5 to 00b1677 Compare November 13, 2025 20:37
@cfm cfm requested a review from Copilot November 13, 2025 20:37
Copilot finished reviewing on behalf of cfm November 13, 2025 20:40
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cfm cfm force-pushed the 7639-apiv2-minor-versioning branch from 00b1677 to f241f6b Compare November 13, 2025 20:47
@cfm cfm changed the title feat(/data): Prefer: securedrop=x limits response shape to minor version 2.x feat: Prefer: securedrop=x limits v2 Journalist API response shape to minor version 2.x Nov 13, 2025
@cfm cfm marked this pull request as ready for review November 13, 2025 20:49
@cfm cfm requested a review from a team as a code owner November 13, 2025 20:49
@cfm cfm moved this from In Progress to Ready For Review in SecureDrop Nov 13, 2025
@cfm cfm assigned legoktm and unassigned cfm Nov 13, 2025
@cfm cfm requested a review from legoktm November 13, 2025 20:52
@cfm cfm force-pushed the 7639-apiv2-minor-versioning branch 2 times, most recently from 7ef0e42 to 88d5991 Compare November 14, 2025 01:39
@cfm cfm changed the title feat: Prefer: securedrop=x limits v2 Journalist API response shape to minor version 2.x feat(api2): Prefer: securedrop=x limits response shape to minor version 2.x Nov 14, 2025
legoktm
legoktm previously approved these changes Nov 14, 2025
Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

Test plan checks out, LGTM. Left two nits, but if you don't want to address them, feel free to merge right away.

@legoktm legoktm moved this from Ready For Review to Under Review in SecureDrop Nov 14, 2025
cfm added 2 commits November 14, 2025 09:50
…sion 2.x

This avoids overloading the "Accept" header's syntax for content
negotiation, since we don't care about its caching semantics.
@cfm cfm force-pushed the 7639-apiv2-minor-versioning branch from 88d5991 to e888fef Compare November 14, 2025 17:51
@cfm cfm requested a review from legoktm November 14, 2025 17:52
Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

Thanks!

@legoktm legoktm added this pull request to the merge queue Nov 17, 2025
Merged via the queue into develop with commit d3e613a Nov 17, 2025
19 checks passed
@github-project-automation github-project-automation bot moved this from Under Review to Done in SecureDrop Nov 17, 2025
@legoktm legoktm deleted the 7639-apiv2-minor-versioning branch November 17, 2025 15:46
@nathandyer nathandyer removed this from SecureDrop Nov 17, 2025
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.

/api/v2/index: let the client specify types to return

3 participants