Skip to content

fix: avoid mutating resource paths in ODataRequestCount#1121

Open
Louis-Stegra wants to merge 2 commits intoSAP:mainfrom
Louis-Stegra:fix/stateful-odata4-count-request-builder
Open

fix: avoid mutating resource paths in ODataRequestCount#1121
Louis-Stegra wants to merge 2 commits intoSAP:mainfrom
Louis-Stegra:fix/stateful-odata4-count-request-builder

Conversation

@Louis-Stegra
Copy link

@Louis-Stegra Louis-Stegra commented Mar 14, 2026

Context

Bug report: #1120

Feature scope:

CountRequestBuilder.toRequest() for OData V4 had the side effect of mutating the shared ODataResourcePath when appending $count to it. Reusing the same builder could therefore produce invalid paths such as $count/$count. This change makes ODataRequestCount copy the incoming resource path before appending extra segments, so repeated toRequest() calls remain stable and do not corrupt later execution.

That fix was inspired by a similar implementation in ODataRequestFunction

Feature scopes:

  • Prevent ODataRequestCount constructors from mutating the provided ODataResourcePath
  • Add a low-level regression test proving ODataRequestCount does not modify the original resource path
  • Add a V4 builder-level regression test proving repeated CountRequestBuilder.toRequest() calls return the same URI

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Documentation updated
  • Release notes updated

@cla-assistant
Copy link

cla-assistant bot commented Mar 14, 2026

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@newtork newtork left a comment

Choose a reason for hiding this comment

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

I would prefer if we fixed it for other request builder(s) too.

From first look it appears we can change the behavior of ODataResourcePath to be immutable by introducing wither-like behavior on the addXyz(..) methods. I.e. not returning this but a new instance.
If we changed it that way, we'll need a sentence in our release notes too 👍

@Louis-Stegra
Copy link
Author

Sounds like a reasonable option. I started with targeted fix, as I don't really know the codebase. If you believe it's better to make ODataResourcePath immutable, I can make that change.

@Louis-Stegra
Copy link
Author

Louis-Stegra commented Mar 16, 2026

Updated PR:

Context

#1120

This change makes ODataResourcePath immutable by returning new instances from addSegment(...) and addParameterToLastSegment(...) instead of mutating the existing path. This fixes stateful request construction in ODataRequestCount and related request builders where shared path instances could previously be modified in place and reused in a corrupted state.

Feature scope:

  • Make ODataResourcePath append/update operations return new path instances
  • Update OData request classes to rely on immutable path behavior
  • Update V4 navigation/request builder code to reassign returned resource paths
  • Add regression tests to ensure request construction no longer mutates caller-owned paths
  • Add a V4 builder-level regression test for repeated CountRequestBuilder.toRequest() calls
  • Add a release note for the ODataResourcePath compatibility change

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Release notes updated

@Louis-Stegra Louis-Stegra requested a review from newtork March 16, 2026 14:51
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