Conversation
github-actions
bot
added
java
andrescrz
force-pushed
the
andrescrz/OPIK-5206-em-workspace-version
branch
from
bd67c06 to
d307bbc
Compare
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
apps/opik-backend/src/test/java/com/comet/opik/infrastructure/auth/RemoteAuthServiceTest.java
Show resolved
Hide resolved
…etermination Implement the auth one-way V2 gate and fallback using the opikVersion field from the EM auth response (OPIK-5170).
Change @JsonCreator fromValue to return null instead of throwing IllegalArgumentException for unknown values. This prevents auth failures when EM is deployed with a newer version value before the backend is updated. The system already handles null opikVersion defensively: auth gate is skipped, entity check runs, fallback returns VERSION_1.
andrescrz
force-pushed
the
andrescrz/OPIK-5206-em-workspace-version
branch
from
d307bbc to
50adc38
Compare
There was a problem hiding this comment.
[Low] No test covers a cache hit where the Redis map is missing the opikVersion key entirely (i.e. entries written before this PR). m.get(OPIK_VERSION_KEY) returns null in that case, which findByValue handles correctly — but a regression test would lock this behavior in and prevent silent breakage if the read path changes.
There was a problem hiding this comment.
Already covered — testCacheAndRetrieveOpikVersion(null) tests the opikVersion=null round-trip, which exercises the same findByValue → Optional.empty() → null code path. The only difference with a pre-PR entry is the input to findByValue (null vs ""), but both are non-matching values that produce the same result. Testing the missing-key case specifically would require direct Redis writes (not black box).
🤖 Reply posted via /address-github-pr-comments
apps/opik-backend/src/test/java/com/comet/opik/infrastructure/auth/RemoteAuthServiceTest.java
Show resolved
Hide resolved
- Add comments explaining the serialize→Map→override→re-serialize pattern in testAuthSuccessful/testSessionAuthSuccessful (bypasses @jsonvalue to inject raw strings like "VERSION_1", "version_unknown")
…rsion (#5975) * [OPIK-5206] [BE] feat: add V1 entity check for alerts in workspace version Add alerts to the workspace version V1 entity check. Alerts without project_id (workspace-level) now signal VERSION_1. Project-scoped alerts do not trigger V1. * test(auth): address PR #5964 review feedback from thiagohora - Add comments explaining the serialize→Map→override→re-serialize pattern in testAuthSuccessful/testSessionAuthSuccessful (bypasses @jsonvalue to inject raw strings like "VERSION_1", "version_unknown")
2 participants
Details
Consume the
opikVersionfield from the EM auth response and propagate it through the auth cache andRequestContextto the workspace version service. This implements the auth one-way V2 gate: when EM indicatesversion_2, always return V2 regardless of entity state (prevents V2→V1 demotion). Defensive design — gracefully degrades when EM doesn't provide the field (null falls through to entity check).opikVersiontoAuthResponse,ValidatedAuthCredentials,AuthCredentials, Redis cache, andRequestContextgetWorkspaceVersionreceivesauthSuggestedVersionas parameter;computeVersionchecks for one-way V2 gate; auth-aware fallback on DB failureCacheService.cache()to acceptAuthCredentialsrecord instead of individual argsRemoteAuthService: factory methods for clean mapping,setCredentialIntoContextencapsulates all context writesOpikVersion.findByValuenow case-insensitive (defensive hardening)Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
Commands run:
mvn test -pl . -Dtest="RemoteAuthServiceTest"— 17 tests, all passmvn test -pl . -Dtest="AuthCredentialsCacheServiceTest"— 12 tests, all passmvn test -pl . -Dtest="RemoteAuthServiceTest,AuthCredentialsCacheServiceTest,AuthServiceImplTest"— 32 tests, all passScenarios validated:
opikVersionnull/V1/V2 stored and retrieved from Redis with full equality assertionopikVersionpropagation from EM response toRequestContext(null, V1, V2 × header/query param workspace)toAuthCredentials()→from(AuthCredentials)preserves all fields via mock CacheService with ArgumentCaptorDocumentation
No documentation updates required — internal API change, no user-facing API changes.