RHCLOUD-45005: Use ResourceType/ReporterType in SchemaRepository and ResourceEvent#1333
Open
snehagunta wants to merge 1 commit intoproject-kessel:mainfrom
Open
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR replaces raw string resource/reporter identifiers with strongly-typed domain types ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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)
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 42 minutes and 59 seconds.Comment |
snehagunta
added a commit
to snehagunta/inventory-api
that referenced
this pull request
May 1, 2026
This method was accidentally included in the ContinuationToken branch. It belongs in PR project-kessel#1333 (ResourceType/ReporterType). Co-authored-by: Cursor <cursoragent@cursor.com>
snehagunta
added a commit
that referenced
this pull request
May 1, 2026
* Use ContinuationToken tiny type for pagination and streaming Replace raw string with model.ContinuationToken in pagination, ReadTuplesItem, LookupObjectsItem, and LookupSubjectsItem. Add DeserializeContinuationToken for trusted reconstruction from gRPC responses and .String() at proto response boundaries. Made-with: Cursor * fix: use ContinuationToken type in tuples pagination conversion The paginationFromProto function was still using *string for the continuation token instead of *model.ContinuationToken, and readTuplesItemToProto was passing ContinuationToken directly to the proto field instead of calling .String(). Made-with: Cursor * refactor: rename 'out' to 'pagination' in paginationFromProto Address review feedback from Rajagopalan-Ranganathan. Co-authored-by: Cursor <cursoragent@cursor.com> * fix: remove SchemaRepositoryKey that belongs in 2D PR This method was accidentally included in the ContinuationToken branch. It belongs in PR #1333 (ResourceType/ReporterType). Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
snehagunta
force-pushed
the
RHCLOUD-45005-tiny-types-resource-reporter-type
branch
from
f7a6d7c to
588fec3
Compare
![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)
internal/data/schema_inmemory.go (1)
266-318:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on schema read errors.
This block treats any read error as “schema missing”. A permission/I/O failure in
loadCommonResourceDataSchemaorloadResourceSchemawill leave the repository partially loaded and silently disable validation for that resource/reporter. Only the not-found case should be ignored.Suggested direction
+import "errors" ... - commonResourceSchema, err := loadCommonResourceDataSchema(resourceLabel, resourceDir) - if err == nil { + commonResourceSchema, err := loadCommonResourceDataSchema(resourceLabel, resourceDir) + if err != nil { + if !errors.Is(err, os.ErrNotExist) { + return nil, err + } + } else { rt, perr := bizmodel.NewResourceType(resourceLabel) if perr != nil { return nil, fmt.Errorf("invalid resource type directory %q: %w", resourceLabel, perr) } err = repository.CreateResourceSchema(ctx, bizmodel.ResourceSchema{ ResourceType: rt, ValidationSchema: validationSchemaFromString(commonResourceSchema), }) if err != nil { return nil, err } } ... - reporterSchema, isReporterSchemaExists, err := loadResourceSchema(resourceLabel, reporterLabel, resourceDir) - if err == nil && isReporterSchemaExists { + reporterSchema, isReporterSchemaExists, err := loadResourceSchema(resourceLabel, reporterLabel, resourceDir) + if err != nil { + return nil, err + } + if isReporterSchemaExists { ... } else { log.Warnf("No schema found for %s:%s", resourceLabel, reporterLabel) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/data/schema_inmemory.go` around lines 266 - 318, The code currently treats any error from loadCommonResourceDataSchema and loadResourceSchema as "schema missing" and continues, which hides I/O/permission failures; change the error handling so that only not-found errors are ignored: for loadCommonResourceDataSchema, if err != nil then if os.IsNotExist(err) (or errors.Is(err, fs.ErrNotExist)) continue, else return the error; for loadResourceSchema, if err != nil then if the error indicates not-found (use os.IsNotExist/errors.Is) log the missing schema and continue, otherwise return the error; keep the existing successful-path behavior that calls bizmodel.NewResourceType/NewReporterType and repository.CreateResourceSchema/CreateReporterSchema and validationSchemaFromString.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/data/schema_inmemory.go`:
- Around line 446-455: findResourceTypeFromJsonKey currently matches jsonKey
against rt.SchemaRepositoryKey() raw, which fails when the cache uses
non-canonical resource labels; split the jsonKey at the first ':' to extract the
resource segment, normalize that resource segment using the same normalization
helper used elsewhere for canonical resource labels (reuse the existing resource
normalization function in bizmodel, e.g.,
NormalizeResourceLabel/NormalizeResourceSegment), then recombine with ':' and
compare against rt.SchemaRepositoryKey()+":" to determine a match; update the
function findResourceTypeFromJsonKey to parse, normalize, and then compare so
reporter cache keys like "RHEL/Host:hbi" or "K8S_CLUSTER:hbi" will match.
---
Outside diff comments:
In `@internal/data/schema_inmemory.go`:
- Around line 266-318: The code currently treats any error from
loadCommonResourceDataSchema and loadResourceSchema as "schema missing" and
continues, which hides I/O/permission failures; change the error handling so
that only not-found errors are ignored: for loadCommonResourceDataSchema, if err
!= nil then if os.IsNotExist(err) (or errors.Is(err, fs.ErrNotExist)) continue,
else return the error; for loadResourceSchema, if err != nil then if the error
indicates not-found (use os.IsNotExist/errors.Is) log the missing schema and
continue, otherwise return the error; keep the existing successful-path behavior
that calls bizmodel.NewResourceType/NewReporterType and
repository.CreateResourceSchema/CreateReporterSchema and
validationSchemaFromString.
🪄 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: 5a284164-d2a5-45e2-8e11-592f6113baf4
📒 Files selected for processing (13)
internal/biz/model/common.gointernal/biz/model/resource_delete_event.gointernal/biz/model/resource_event.gointernal/biz/model/resource_report_event.gointernal/biz/model/schema_repository.gointernal/biz/model/schema_service.gointernal/biz/model_legacy/outboxevents.gointernal/biz/model_legacy/outboxevents_test.gointernal/biz/usecase/resources/resource_service.gointernal/biz/usecase/resources/resource_service_test.gointernal/data/schema_inmemory.gointernal/data/schema_inmemory_test.gointernal/service/resources/kesselinventoryservice_test.go
Contributor
Author
Sanity Test ResultsBranch: Commit: Full test output |
snehagunta
commented
May 1, 2026
| reporterType := cmd.ReporterType.String() | ||
|
|
||
| if resourceType == "" { | ||
| if cmd.ResourceType.String() == "" { |
Contributor
Author
There was a problem hiding this comment.
We do not need to do this validation since this check is already done during construction of the type. That is the advantage of using the type.
snehagunta
force-pushed
the
RHCLOUD-45005-tiny-types-resource-reporter-type
branch
from
5d2172e to
25be5b7
Compare
| bizmodel "github.com/project-kessel/inventory-api/internal/biz/model" | ||
| ) | ||
|
|
||
| func dRT(s string) bizmodel.ResourceType { |
Contributor
There was a problem hiding this comment.
nitpick: both these method names can be a bit verbose, not good for readability
Rajagopalan-Ranganathan
approved these changes
May 1, 2026
Contributor
|
/lgtm - one nitpick comment and if time permits will do another review in detail. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/data/schema_inmemory.go (1)
446-455:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize the resource segment in JSON keys before matching.
The current implementation compares
NormalizeResourceType(rt) + ":"against the rawjsonKey. If the JSON cache contains non-canonical keys like"RHEL/Host:hbi"or"K8S_CLUSTER:hbi", the prefix check will fail becauseNormalizeResourceTypereturns lowercase with underscores (e.g.,"rhel_host:"), which won't match the raw key.Parse and normalize the resource segment from
jsonKeybefore comparison.🔧 Proposed fix
func findResourceTypeFromJsonKey(jsonKey string, resourceTypes []bizmodel.ResourceType) (bizmodel.ResourceType, bool) { + resourceSegment, _, ok := strings.Cut(jsonKey, ":") + if !ok { + return bizmodel.ResourceType(""), false + } + normalizedSegment := NormalizeResourceType(bizmodel.DeserializeResourceType(resourceSegment)) + for _, rt := range resourceTypes { - prefix := NormalizeResourceType(rt) + ":" - if strings.HasPrefix(jsonKey, prefix) { + if NormalizeResourceType(rt) == normalizedSegment { return rt, true } } return bizmodel.ResourceType(""), false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/data/schema_inmemory.go` around lines 446 - 455, The prefix check should normalize the resource segment from jsonKey before comparing; instead of checking strings.HasPrefix(jsonKey, NormalizeResourceType(rt)+":"), split jsonKey at the first ':' to extract the resource segment (e.g., seg := strings.SplitN(jsonKey, ":", 2)[0]), convert that segment into a bizmodel.ResourceType and run it through NormalizeResourceType (segNorm := NormalizeResourceType(bizmodel.ResourceType(seg))), then inside the loop compare segNorm == NormalizeResourceType(rt) and return rt when equal; if there is no ':' or no match, return the zero value and false as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/schema/schema.go`:
- Around line 121-125: The code incorrectly calls bizmodel.NewResourceType for
reporter directories; replace the constructor call with
bizmodel.NewReporterType(reporter.Name()) and then normalize appropriately
(either add/use a NormalizeReporterType in the data package or pass the reporter
type's string to the existing data.NormalizeResourceType if reporter
normalization is equivalent). Update the variable name (rpt → rptr or similar)
to reflect a reporter type and ensure subsequent code uses the
reporter-normalized value instead of data.NormalizeResourceType(rpt) if you add
a distinct data.NormalizeReporterType function.
---
Duplicate comments:
In `@internal/data/schema_inmemory.go`:
- Around line 446-455: The prefix check should normalize the resource segment
from jsonKey before comparing; instead of checking strings.HasPrefix(jsonKey,
NormalizeResourceType(rt)+":"), split jsonKey at the first ':' to extract the
resource segment (e.g., seg := strings.SplitN(jsonKey, ":", 2)[0]), convert that
segment into a bizmodel.ResourceType and run it through NormalizeResourceType
(segNorm := NormalizeResourceType(bizmodel.ResourceType(seg))), then inside the
loop compare segNorm == NormalizeResourceType(rt) and return rt when equal; if
there is no ':' or no match, return the zero value and false as before.
🪄 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: f6c7ab4d-8e8e-483c-ada5-3a401963d347
📒 Files selected for processing (5)
cmd/schema/schema.gointernal/biz/usecase/resources/resource_service.gointernal/biz/usecase/resources/resource_service_test.gointernal/data/schema_inmemory.gointernal/data/schema_inmemory_test.go
snehagunta
commented
May 1, 2026
| ReporterType() string | ||
| ResourceType() ResourceType | ||
| ReporterType() ReporterType | ||
| ReporterInstanceId() string |
Contributor
Author
There was a problem hiding this comment.
These 2 need to be updated too. I'll do it in a follow on PR
snehagunta
force-pushed
the
RHCLOUD-45005-tiny-types-resource-reporter-type
branch
2 times, most recently
from
9e36cde to
614266e
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/service/resources/kesselinventoryservice_test.go (1)
3947-3948: 💤 Low valueConsider adding
t.Helper()tonewFakeSchemaRepository.
buildReporterResourceKey(line 3146) already callst.Helper(). Adding it here would makerequire.NoErrorfailures report the callsite (e.g.,newTestUsecase) rather than the line inside this helper, which aids debugging.♻️ Proposed change
func newFakeSchemaRepository(t *testing.T) model.SchemaRepository { + t.Helper() schemaRepository := data.NewInMemorySchemaRepository()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/resources/kesselinventoryservice_test.go` around lines 3947 - 3948, Add t.Helper() to the test helper newFakeSchemaRepository so test failures inside it are reported at the caller site; locate the function newFakeSchemaRepository(t *testing.T) in internal/service/resources/kesselinventoryservice_test.go and add a single call to t.Helper() at the top of that function before creating the schemaRepository (so require.NoError and similar assertions point to the calling test like newTestUsecase rather than inside the helper).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/service/resources/kesselinventoryservice_test.go`:
- Around line 3947-3948: Add t.Helper() to the test helper
newFakeSchemaRepository so test failures inside it are reported at the caller
site; locate the function newFakeSchemaRepository(t *testing.T) in
internal/service/resources/kesselinventoryservice_test.go and add a single call
to t.Helper() at the top of that function before creating the schemaRepository
(so require.NoError and similar assertions point to the calling test like
newTestUsecase rather than inside the helper).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 671329fc-3531-4618-a476-a8bf0dd0ddc2
📒 Files selected for processing (4)
cmd/schema/schema.gointernal/data/schema_inmemory.gointernal/data/schema_inmemory_test.gointernal/service/resources/kesselinventoryservice_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/data/schema_inmemory.go
- internal/data/schema_inmemory_test.go
snehagunta
force-pushed
the
RHCLOUD-45005-tiny-types-resource-reporter-type
branch
2 times, most recently
from
048f435 to
9aba36c
Compare
… and events Replace raw string usage of resource type and reporter type with domain-specific tiny types throughout the schema repository, schema service, resource events, outbox events, and related tests. - Update SchemaRepository interface and InMemorySchemaRepository to use ResourceType/ReporterType parameters and return types - Update ResourceEvent interface to return ResourceType/ReporterType - Update SchemaService to accept ResourceType/ReporterType - Update resource_service validation to pass types directly - Add NormalizeReporterType matching existing NormalizeResourceType - Convert loadResourceSchema/loadCommonResourceDataSchema to accept tiny types - Update cmd/schema to use proper constructors for both types - Use .String() at external boundaries (event serialization, map keys, file paths, error messages) Co-authored-by: Cursor <cursoragent@cursor.com>
snehagunta
force-pushed
the
RHCLOUD-45005-tiny-types-resource-reporter-type
branch
from
9aba36c to
7ad9481
Compare
Contributor
Author
Sanity Test Results — All 16/16 PASSEDNamespace: |
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.
Summary
stringwithmodel.ResourceTypeandmodel.ReporterTypeinSchemaRepositoryinterface,SchemaService, andResourceEventinterfaceSchemaRepositoryKey()method toResourceTypefor normalized map lookupsResourceReportEventandResourceDeleteEventto return typed values.String()on typed gettersTest plan
go build ./...)Made with Cursor
Summary by CodeRabbit
Refactor
Bug Fixes
Tests