Repository navigation
Conversation
Resolve the lint-only Error Prone findings across the library modules: - MissingSummary: add summary sentences to Javadocs that previously contained only an @author tag (api key types, OAuth2 webclient filters, configurers, resource identifier, DCR manager test). - StringCaseLocaleUsage: parse the Cache-Control header with toLowerCase(Locale.ROOT). - CanonicalDuration: suppress on the session timeout field, keeping the explicit Duration.ofHours(48) for readability. - ExtendsObject: drop the redundant `extends Object` type bound. - EmptyCatch: document why the ClassNotFoundException is ignored. - InlineMeSuggester: suppress on the deprecated 3-arg DCR manager constructor (@InlineMe is not on the classpath). - TypeParameterUnusedInFormals: suppress on ApiKeyEntity.copy() and on getOptionalBean, preserving call-site ergonomics and (for the latter) fidelity with the upstream Spring Security helper. - StringSplitter: suppress on the JWT-payload split in the authorization server test (no Guava on the classpath, behaviour is fine). - UnusedVariable / DefaultCharset: remove the unused SECRET test field and its now-unused imports. Signed-off-by: Anil Kumar Veldurthi <anil.veldurthi@gmail.com>
…munitygh-72 - MissingSummary: add summary sentences to the sample application Javadocs. - UnusedMethod: remove the dead findUniqueClientRegistration helper and its now-unused imports. Signed-off-by: Anil Kumar Veldurthi <anil.veldurthi@gmail.com>
…ring-ai-communitygh-72 ApiKeyImpl.from() previously split the "id.secret" string on every "." and kept only the first two segments, silently truncating any secret that contained a ".". A caller presenting the full, correct key would then fail authentication. This changes the parsing behavior: the id is now everything before the first ".", and the secret is the entire remainder, so secrets containing "." are preserved (previously silently truncated). The existing contains(".") guard keeps the separator index valid. This is a deliberate behavior change in the API-key authentication path, not merely the StringSplitter lint cleanup that surfaced it. Signed-off-by: Anil Kumar Veldurthi <anil.veldurthi@gmail.com>
Kehrlann
requested changes
Jun 7, 2026
Kehrlann
left a comment
Kehrlann
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for your contribution!
We have Error Prone in the project for NullAway support, were are not really leveraging the rest of error prone. I'd rather not introduce @SuppressWarnings - in the future, we may have a difference checker for JSpecify,
Let's:
- Get rid of all the suppressions
- Remove the current warnings from error prone in the maven config
Comment on lines
+41
to
+44
| int separatorIndex = apiKey.indexOf('.'); | ||
| var id = apiKey.substring(0, separatorIndex); | ||
| var secret = apiKey.substring(separatorIndex + 1); | ||
| return new ApiKeyImpl(id, secret); |
Collaborator
There was a problem hiding this comment.
Given that the "surprising behavior" is about ".".split("\\."), I'd say we are safe here and we should revert this.
Contributor
Author
There was a problem hiding this comment.
Will do, reverting to split. One check before I do: can the secret half ever contain a .? With split("\\.") a key like id.ab.cd parses the secret as ab (drops cd), whereas the indexOf version kept it as ab.cd. If secrets are dot-free by construction, the revert is clean, and I'll drop the regression test with it.
…curity/server/apikey/ApiKeyImplTests.java Co-authored-by: Daniel Garnier-Moiroux <daniel.garnier-moiroux@broadcom.com>
Signed-off-by: Anil Kumar Veldurthi <anil.veldurthi@gmail.com>
Contributor
Author
|
Thanks @Kehrlann, review addressed:
One suppression left in place: |
Collaborator
|
Conflicts resolved and merged in e525488 Thanks for your contribution! |
Contributor
Author
|
Thank you for the review and merge @Kehrlann, learned a lot from the feedback. Looking forward to contributing more. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves the Error Prone / NullAway warnings from gh-72 across the reactor - the build now reports zero warnings (
./mvnw clean verifygreen). Mostly mechanical (Javadoc summaries,Locale.ROOT, unused symbols, and a few@SuppressWarningswhere the flagged code is intentional).One behavior change worth flagging:
ApiKeyImpl.from()no longer truncates a secret containing.- it now splits on the first.only, with a regression test. Happy to reject malformed keys instead if that's the intended format. Also includes thesamples/warnings; glad to remove them if gh-72 is library-only.