Skip to content

HDDS-14104. Refactor RequestContext creation#9493

Merged
adoroszlai merged 4 commits into
apache:masterfrom
Russole:HDDS-14104
Dec 13, 2025
Merged

HDDS-14104. Refactor RequestContext creation#9493
adoroszlai merged 4 commits into
apache:masterfrom
Russole:HDDS-14104

Conversation

@Russole
Copy link
Copy Markdown
Contributor

@Russole Russole commented Dec 13, 2025

What changes were proposed in this pull request?

Refactored RequestContext to use a builder-only construction pattern.

  • Removed multiple public constructors and redundant getBuilder helpers from RequestContext.
  • Adopted builder-only construction by making the Builder constructor private.
  • Declared RequestContext and its Builder as final.
  • Updated tests to use RequestContext.newBuilder().

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14104

How was this patch tested?

All CI checks passed.

Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @Russole for the patch. The change in RequestContext looks good. TestRequestContext can be simplified.

Comment on lines +250 to +251
.setIp(null)
.setHost(null)
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.

nit: null is the default value in the builder for these properties, so this can be removed.

Suggested change
.setIp(null)
.setHost(null)

.setSessionPolicy(policy)
.build();
assertTrue(context.isRecursiveAccessCheck(), "recursiveAccessCheck should be true");
assertEquals(policy, context.getSessionPolicy(), "sessionPolicy should be set via constructor");
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.

Looks like testSessionPolicy() has been testing sessionPolicy for various ways of creating RequestContext. Now constructor is private and getBuilder no longer exists, so all variants are testing the builder. We can remove the parts that perform duplicate verifications, and simplify this to:

  @Test
  void testSessionPolicy() {
    RequestContext.Builder builder = RequestContext.newBuilder();
    assertNull(builder.build().getSessionPolicy(), "default value");

    final String policy = "{\"Statement\":[]}";
    builder.setSessionPolicy(policy);
    assertEquals(policy, builder.build().getSessionPolicy());
  }

Comment on lines 54 to 57
RequestContext.Builder builder = RequestContext.newBuilder();

assertFalse(builder.build().isRecursiveAccessCheck(),
"Wrongly sets recursive flag value");
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.

testRecursiveAccessFlag() has also been testing constructor, builder and getBuilder. Now there are only 3 possible combinations:

  • default value (without explicit setRecursiveAccessCheck)
  • setRecursiveAccessCheck(false)
  • setRecursiveAccessCheck(true)

So we can simplify this test case further:

  @Test
  void testRecursiveAccessFlag() {
    RequestContext.Builder builder = RequestContext.newBuilder();

    assertFalse(builder.build().isRecursiveAccessCheck(), "default value");

    builder.setRecursiveAccessCheck(true);
    assertTrue(builder.build().isRecursiveAccessCheck());

    builder.setRecursiveAccessCheck(false);
    assertFalse(builder.build().isRecursiveAccessCheck());
  }

getUserRequestContext() and baseBuilder() can be removed, as well as unused imports.

@adoroszlai adoroszlai changed the title Hdds-14104. Refactor RequestContext creation HDDS-14104. Refactor RequestContext creation Dec 13, 2025
@Russole
Copy link
Copy Markdown
Contributor Author

Russole commented Dec 13, 2025

Thanks @adoroszlai for the review and suggestions.

@Russole Russole requested a review from adoroszlai December 13, 2025 09:33
@adoroszlai adoroszlai merged commit 59ad0a1 into apache:master Dec 13, 2025
43 checks passed
@adoroszlai
Copy link
Copy Markdown
Contributor

Thanks @Russole for updating the patch.

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