Skip to content

Allow current=true filter on GET /v3/droplets to return only current droplets across all apps - #5360

Open
WeiQuan0605 wants to merge 3 commits into
cloudfoundry:mainfrom
sap-contributions:add-current-filter-to-global-droplets-endpoint
Open

Allow current=true filter on GET /v3/droplets to return only current droplets across all apps#5360
WeiQuan0605 wants to merge 3 commits into
cloudfoundry:mainfrom
sap-contributions:add-current-filter-to-global-droplets-endpoint

Conversation

@WeiQuan0605

Copy link
Copy Markdown
Contributor
  • A short explanation of the proposed change:

    GET /v3/droplets now accepts current=true as a query parameter, returning only droplets that are set as the current droplet of an app. Previously this filter was only available on the app-nested endpoint GET /v3/apps/:guid/droplets?current=true.

  • An explanation of the use cases your change solves

    Clients that need to display stack information for multiple apps (e.g. to warn about deprecated stacks) previously had to make one request per app to fetch the current droplet. With this change, they can fetch current droplets for all apps in a single request by combining with existing filters:
    GET /v3/droplets?current=true&app_guids=guid1,guid2,...

    This reduces N+1 API requests to 2 requests total.

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests


private

def not_app_nested_request

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we convert that method into a package-specific validator rather than removing it?
not_app_nested_request gated all routes where app_guid is absent, that's both the global GET /v3/droplets route and the package-nested GET /v3/packages/:guid/droplets route. Removing it enables current=true on the global route as intended, but also silently enables it on the package route, which flips that endpoint from returning 422 Unknown query parameter to 200 with a join executed. That's outside the PR's stated scope and untested. A package_nested_request validator (firing on package_guid.present?) would still reject current there while allowing it on the global route.

@@ -125,10 +125,15 @@ module VCAP::CloudController

context 'when the query is not nested under an app' do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we add a test asserting current is still rejected on the package-nested route (package_guid present, app_guid absent)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added

end
end
end
end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

current=true combined with organization_guids or space_guids isn't covered, those add a second join to apps (apps_orgs/apps_spaces alongside the new apps_current). Could we add fetcher tests for those two combinations?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added

space_guids
app_guids
organization_guids
current

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This moves current into the valid-params list, but there's no request-level test that actually calls GET /v3/droplets?current=true and asserts the response returns only current droplets. The filtering is covered at the fetcher unit level, but a request spec for the happy path would cover the full controller→message→fetcher→presenter path. Non-blocking, but worth adding.

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.

2 participants