Skip to content
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

Removing assertions from code and replacing with relevant exceptions #1085

Merged
merged 13 commits into from
Oct 22, 2024

Conversation

asloobq
Copy link
Contributor

@asloobq asloobq commented Oct 14, 2024

Refactors

  • move test method out of UIDOperatorService.java
  • moving error message string literals to constants

TODO

  • - run E2E tests
  • integration deploy and test

return this.processPartitions(iuc);
} else {
// array size cannot be a negative value
assert numPartitions == 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this condition is always true

return this.processDeltas(iuc);
} else if (numPartitions > 1) {
// should not load more than 1 partition at a time, unless during service bootstrap
assert this.iteration == 0;
if (this.iteration != 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pom.xml Outdated
@@ -6,7 +6,7 @@

<groupId>com.uid2</groupId>
<artifactId>uid2-operator</artifactId>
<version>5.40.86</version>
<version>5.40.87-alpha-110-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be removed from the final merge?

Comment on lines 37 to 40
assert item1 != null;
assert item2 != null;
assert item3 != null;
if (item1 == null || item2 == null || item3 == null) {
throw new NullPointerException();
}
Copy link
Contributor

@mcollins-ttd mcollins-ttd Oct 18, 2024

Choose a reason for hiding this comment

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

The line number in the stack trace won't tell us which of the items is null. Suggest instead

this.item1 = Objects.requireNonNull(item1);
this.item2 = Objects.requireNonNull(item2);
this.item3 = Objects.requireNonNull(item3);

Same for the other Tuple.

@@ -88,7 +88,9 @@ public void handleMessage(Message message) {
return;
}

assert messageItem != null;
if (messageItem == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can readValue return null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it won't return a null value. If the input string is null it throws an IllegalArgumentException and Json*Exception for bad json string. I'll remove this check. Thanks

Comment on lines 156 to 161
try {
V2RequestUtil.handleRefreshTokenInResponseBody(jsonBody, keyManager, IdentityScope.UID2);
fail("IllegalArgumentException not thrown");
} catch (IllegalArgumentException e) {
assertEquals("Generated refresh token's length=168 is not equal to=388", e.getMessage());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

Suggested change
try {
V2RequestUtil.handleRefreshTokenInResponseBody(jsonBody, keyManager, IdentityScope.UID2);
fail("IllegalArgumentException not thrown");
} catch (IllegalArgumentException e) {
assertEquals("Generated refresh token's length=168 is not equal to=388", e.getMessage());
}
IllegalArgumentException e = assertThrowsExactly(
IllegalArgumentException.class,
() -> V2RequestUtil.handleRefreshTokenInResponseBody(jsonBody, keyManager, IdentityScope.UID2));
assertEquals("Generated refresh token's length=168 is not equal to=388", e.getMessage());

@@ -1363,9 +1353,9 @@ private InputUtil.InputVal getTokenInputV1(RoutingContext rc) {
return null;
}

private boolean checkForInvalidTokenInput(InputUtil.InputVal input, RoutingContext rc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

good that you renamed checkForInvalidTokenInput as after reading the original codes and what bool it returns, this method name is highly deceptive!

@asloobq asloobq merged commit ce1ab8b into main Oct 22, 2024
4 checks passed
@asloobq asloobq deleted the aaq-UID2-3393-remove-assertions branch October 22, 2024 17:29
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.

3 participants