RHCLOUD-45005: Normalize ResourceType/ReporterType/ReporterInstanceId at construction#1338
Open
snehagunta wants to merge 2 commits intoproject-kessel:mainfrom
Open
Conversation
… at construction Move lowercasing (and slash-to-underscore for ResourceType) into New* constructors so values are normalized from the moment of creation. This makes Serialize() a passthrough and allows == instead of EqualFold for key matching. Delete NormalizeResourceType, NormalizeReporterType, and NormalizeType functions that are now redundant. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
📝 WalkthroughWalkthroughType normalization responsibility is consolidated into ChangesType Normalization Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/schema/schema.go (1)
89-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail preload on invalid schema names instead of silently skipping them.
These branches now call constructors that can fail, but they still just
continue. A typoed resource/reporter directory will now disappear fromschema_cache.jsonwhile the command still reports success, which makes cache corruption very hard to spot. This should return an error and fail the preload instead of silently dropping the entry. The same fail-open pattern is worth fixing forencodeYAMLToBase64(...)below as well.Suggested direction
rt, err := bizmodel.NewResourceType(resource.Name()) if err != nil { - continue + return fmt.Errorf("invalid resource schema directory %q: %w", resource.Name(), err) } resourceType := rt.String() @@ rpt, err := bizmodel.NewReporterType(reporter.Name()) if err != nil { - continue + return fmt.Errorf("invalid reporter schema directory %q for resource %q: %w", reporter.Name(), resourceType, err) } reporterType := rpt.String()Also applies to: 120-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/schema/schema.go` around lines 89 - 93, The code currently swallows constructor errors by continuing when bizmodel.NewResourceType(resource.Name()) fails (used to build resourceType via rt.String()), and likewise ignores errors from encodeYAMLToBase64; change both spots to propagate the error and fail preload instead of continuing silently—return the error (or wrap with context) from the surrounding function when NewResourceType or encodeYAMLToBase64 returns an error so the command fails and the cache isn't corrupted; update the branches that call bizmodel.NewResourceType and encodeYAMLToBase64 to stop using `continue` and instead return the error with clear context (include resource.Name() and which operation failed).
🧹 Nitpick comments (1)
internal/biz/model/common.go (1)
255-295: ⚡ Quick winKeep deserialized type values canonical.
Serialize()is now a passthrough, butDeserializeResourceType,DeserializeReporterType, andDeserializeReporterInstanceIdstill bypass these constructors.DeserializeResource(...)later hydrates snapshots through those helpers, so any non-canonical stored/imported value can survive in memory and then miss the new exact key comparisons infindReporterResourceToUpdateByKey. Normalize in the deserialize path too, or keep serialization canonical until that invariant is guaranteed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/biz/model/common.go` around lines 255 - 295, Deserialize functions (DeserializeResourceType, DeserializeReporterType, DeserializeReporterInstanceId) currently produce raw strings and can leave non-canonical values in memory; update those deserializers to normalize input by trimming/low-casing and/or calling the respective constructors (NewResourceType, NewReporterType, NewReporterInstanceId) so deserialized values match the canonical format produced by Serialize/SerializeString and won't break exact-key lookups like findReporterResourceToUpdateByKey; ensure errors from the constructors are handled/propagated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/biz/model/resource.go`:
- Around line 315-323: The code currently compares LocalResourceId with a
case-sensitive equality (storedKey.LocalResourceId().Serialize() ==
key.LocalResourceId().Serialize()), which breaks prior behavior; change this to
a case-insensitive comparison (e.g. use strings.EqualFold on
storedKey.LocalResourceId().Serialize() and key.LocalResourceId().Serialize())
or normalize/canonicalize LocalResourceId the same way you do
ResourceType/ReporterType before comparing so that LocalResourceId matching
remains case-insensitive in the logic that returns reporter in this function.
---
Outside diff comments:
In `@cmd/schema/schema.go`:
- Around line 89-93: The code currently swallows constructor errors by
continuing when bizmodel.NewResourceType(resource.Name()) fails (used to build
resourceType via rt.String()), and likewise ignores errors from
encodeYAMLToBase64; change both spots to propagate the error and fail preload
instead of continuing silently—return the error (or wrap with context) from the
surrounding function when NewResourceType or encodeYAMLToBase64 returns an error
so the command fails and the cache isn't corrupted; update the branches that
call bizmodel.NewResourceType and encodeYAMLToBase64 to stop using `continue`
and instead return the error with clear context (include resource.Name() and
which operation failed).
---
Nitpick comments:
In `@internal/biz/model/common.go`:
- Around line 255-295: Deserialize functions (DeserializeResourceType,
DeserializeReporterType, DeserializeReporterInstanceId) currently produce raw
strings and can leave non-canonical values in memory; update those deserializers
to normalize input by trimming/low-casing and/or calling the respective
constructors (NewResourceType, NewReporterType, NewReporterInstanceId) so
deserialized values match the canonical format produced by
Serialize/SerializeString and won't break exact-key lookups like
findReporterResourceToUpdateByKey; ensure errors from the constructors are
handled/propagated consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 04980e7d-c962-475c-9a91-ec1e46473b6c
📒 Files selected for processing (8)
cmd/schema/schema.gointernal/biz/model/common.gointernal/biz/model/resource.gointernal/biz/model/schema_service_test.gointernal/data/schema_inmemory.gointernal/data/schema_inmemory_test.gointernal/service/resources/kesselinventoryservice.gointernal/service/resources/kesselinventoryservice_test.go
💤 Files with no reviewable changes (1)
- internal/data/schema_inmemory.go
Comment on lines
+315
to
323
| if storedKey.LocalResourceId().Serialize() == key.LocalResourceId().Serialize() && | ||
| storedKey.ResourceType().Serialize() == key.ResourceType().Serialize() && | ||
| storedKey.ReporterType().Serialize() == key.ReporterType().Serialize() { | ||
|
|
||
| searchReporterInstanceId := key.ReporterInstanceId().Serialize() | ||
| storedReporterInstanceId := storedKey.ReporterInstanceId().Serialize() | ||
|
|
||
| if searchReporterInstanceId == "" || strings.EqualFold(storedReporterInstanceId, searchReporterInstanceId) { | ||
| if searchReporterInstanceId == "" || storedReporterInstanceId == searchReporterInstanceId { | ||
| return reporter, nil |
Contributor
There was a problem hiding this comment.
Don't make LocalResourceId case-sensitive here.
ResourceType, ReporterType, and ReporterInstanceId are now canonicalized, but LocalResourceId still is not. Switching this field from EqualFold to == changes behavior, not just implementation: a resource reported as i-ABC will no longer match an update/delete for i-abc, even though that worked before. Keep exact comparisons only for the normalized fields, or explicitly normalize LocalResourceId too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/biz/model/resource.go` around lines 315 - 323, The code currently
compares LocalResourceId with a case-sensitive equality
(storedKey.LocalResourceId().Serialize() == key.LocalResourceId().Serialize()),
which breaks prior behavior; change this to a case-insensitive comparison (e.g.
use strings.EqualFold on storedKey.LocalResourceId().Serialize() and
key.LocalResourceId().Serialize()) or normalize/canonicalize LocalResourceId the
same way you do ResourceType/ReporterType before comparing so that
LocalResourceId matching remains case-insensitive in the logic that returns
reporter in this function.
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
1 participant
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.
Summary
/->_for ResourceType) intoNew*constructors so values are normalized from the moment of creationSerialize()to passthrough (no longer needs to lowercase)strings.EqualFoldwith==infindReporterResourceToUpdateByKeysince all values are now pre-normalizedNormalizeResourceType,NormalizeReporterTypefromschema_inmemory.goandNormalizeTypefromkesselinventoryservice.gocmd/schema/schema.goto use constructor normalization directlyExternal impact
Kafka CloudEvents will now emit lowercase
resource_type,reporter_type, andreporter_instance_idin event metadata. For the known downstream client (HBI), all values are already lowercase so this is a no-op. See PR discussion for client verification.Test plan
schema_inmemory_test.gogo build ./...succeedsMade with Cursor
Summary by CodeRabbit