RHCLOUD-45005: Use ResourceType/ReporterType in SchemaRepository and ResourceEvent#1332
Closed
snehagunta wants to merge 4 commits intoproject-kessel:mainfrom
Closed
Conversation
Add Decrement() method to Version type with underflow protection, and update TestCalculateTuples to use Version type instead of raw uint. Changes: - Add Version.Decrement() method that returns 0 when already at 0 - Update schema_service_test.go to construct Version values with NewVersion() - Completes the Version type conversion started in previous commits This is a follow-up to the Version tiny type conversion (2A) to ensure all tests use the domain type consistently. Related: RHCLOUD-45005 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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 (1)
📝 WalkthroughWalkthroughThis PR introduces strong typing throughout the codebase by replacing primitive Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 39 minutes and 35 seconds.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:
|
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/resources/kesselinventoryservice.go (1)
301-309:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not ignore continuation-token parse failures in request conversion.
model.NewContinuationTokenerrors are currently discarded, which can silently turn invalid input into an empty/zero token and alter pagination behavior.💡 Proposed fix
-func paginationFromProto(p *pb.RequestPagination) *model.Pagination { +func paginationFromProto(p *pb.RequestPagination) (*model.Pagination, error) { if p == nil { - return nil + return nil, nil } - out := &model.Pagination{Limit: p.Limit} + out := &model.Pagination{Limit: p.Limit} if p.ContinuationToken != nil { - ct, _ := model.NewContinuationToken(*p.ContinuationToken) + ct, err := model.NewContinuationToken(*p.ContinuationToken) + if err != nil { + return nil, fmt.Errorf("invalid continuation token: %w", err) + } out.Continuation = &ct } - return out + return out, nil }- return resources.LookupObjectsCommand{ + pagination, err := paginationFromProto(request.Pagination) + if err != nil { + return resources.LookupObjectsCommand{}, fmt.Errorf("invalid pagination: %w", err) + } + return resources.LookupObjectsCommand{ ObjectType: objectType, Relation: relation, Subject: subjectRef, - Pagination: paginationFromProto(request.Pagination), + Pagination: pagination, Consistency: consistencyFromProto(request.GetConsistency()), }, nil- return resources.LookupSubjectsCommand{ + pagination, err := paginationFromProto(request.Pagination) + if err != nil { + return resources.LookupSubjectsCommand{}, fmt.Errorf("invalid pagination: %w", err) + } + return resources.LookupSubjectsCommand{ Resource: resourceRef, Relation: relation, SubjectType: model.NewRepresentationTypeRequired(subjectResType, subjectReporter), SubjectRelation: subjectRelation, - Pagination: paginationFromProto(request.Pagination), + Pagination: pagination, Consistency: consistencyFromProto(request.GetConsistency()), }, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/resources/kesselinventoryservice.go` around lines 301 - 309, The paginationFromProto function currently swallows errors from model.NewContinuationToken which can turn invalid continuation tokens into a zero value; change paginationFromProto to return (*model.Pagination, error), call model.NewContinuationToken and check its error, and if parsing fails return nil and a descriptive error (e.g., "invalid continuation token: ..."); update all callers of paginationFromProto to handle the new error and propagate or convert it to the appropriate gRPC/HTTP error response so invalid tokens aren’t silently accepted.
🧹 Nitpick comments (2)
internal/biz/model/common.go (1)
102-119: 💤 Low valueContinuationToken constructor always returns nil error.
NewContinuationTokenunconditionally returnsnilfor the error, making the error return value unused. This is inconsistent with other similar constructors (e.g.,NewConsistencyToken,NewLockToken) that validate non-empty input.If empty continuation tokens are intentionally allowed (e.g., for initial pagination requests), consider either:
- Removing the error return since it's never used
- Documenting that empty tokens are valid
This appears intentional based on pagination semantics, so approving as-is but flagging for clarity.
♻️ Suggested simplification (if empty is always valid)
-func NewContinuationToken(token string) (ContinuationToken, error) { - return ContinuationToken(token), nil -} +func NewContinuationToken(token string) ContinuationToken { + return ContinuationToken(token) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/biz/model/common.go` around lines 102 - 119, The NewContinuationToken function currently returns an unused error; if empty continuation tokens are valid, change NewContinuationToken to return only (ContinuationToken) by removing the error from its signature and implementation, update all call sites to accept a single-return value, and adjust related helpers (e.g., any places that call NewContinuationToken) while leaving ContinuationToken.String, Serialize, and DeserializeContinuationToken unchanged; alternatively, if empty tokens must be rejected, add validation in NewContinuationToken to return an error for empty input and ensure callers handle the error.internal/data/fake_resource_repository.go (1)
256-311: ⚡ Quick winFake repository doesn't respect
operationTypefor previous version lookup.The real repository (per context snippet from
resource_repository.go:313-318) only queries for the previous version (cv-1) whenoperationTypeis notCreated:if operationType.OperationType() == bizmodel.OperationTypeCreated { query = query.Where("cr.version = ?", cv) } else { query = query.Where("(cr.version = ? OR cr.version = ?)", cv, cv-1) }However, this fake implementation always attempts to look up
cv-1(lines 294-307) regardless ofoperationType. This could cause test behavior to diverge from production behavior forCreatedoperations.♻️ Proposed fix to respect operationType
if entry, ok := versionMap[cv]; ok { v := bizmodel.NewVersion(entry.commonVersion) var err error current, err = bizmodel.NewRepresentations( bizmodel.Representation(cloneJsonObject(entry.commonData)), &v, nil, nil, ) if err != nil { return nil, nil, err } } - if cv > 0 { + // Only look up previous version for non-Created operations (matching real repository behavior) + if cv > 0 && operationType.OperationType() != bizmodel.OperationTypeCreated { if entry, ok := versionMap[cv-1]; ok { v := bizmodel.NewVersion(entry.commonVersion) var err error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/data/fake_resource_repository.go` around lines 256 - 311, The fake implementation of FindCurrentAndPreviousVersionedRepresentations ignores operationType and always tries to load the previous version; update fakeResourceRepository.FindCurrentAndPreviousVersionedRepresentations to only attempt the cv-1 lookup when operationType.OperationType() != bizmodel.OperationTypeCreated (and cv > 0). Concretely, wrap the block that checks versionMap[cv-1] and constructs previous (the code using cv-1, v := bizmodel.NewVersion(...), and bizmodel.NewRepresentations(...)) in a conditional that checks operationType.OperationType() and cv>0, matching the real repo behavior.
🤖 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/biz/usecase/resources/resource_service.go`:
- Around line 177-182: Save currently diverges between cmd.TransactionId and the
local request-scoped txid (used in createResource/updateResource and outbox),
and the retry path rewrites cmd.TransactionId without updating the captured
txid; fix by computing a single effective transaction ID up front (e.g.
effectiveTxID := first non-nil of cmd.TransactionId and request txid or generate
one) and use that same effectiveTxID everywhere: set cmd.TransactionId =
effectiveTxID before any retries, pass effectiveTxID into uc.createResource and
uc.updateResource calls and into the outbox invocation, or alternatively rename
and separate concepts explicitly (e.g. IdempotencyKey vs OutboxCorrelationId)
and wire them consistently so resource model (cmd.TransactionId), the local txid
variable, and outbox events always carry the same identifier.
In `@internal/data/resource_repository.go`:
- Around line 313-317: The query builds a previous-version predicate using cv-1
which underflows when currentCommonVersion is 0; update the branch that sets
query (around the use of currentCommonVersion.Uint() and
operationType.OperationType() == bizmodel.OperationTypeCreated) to only include
the "(cr.version = ? OR cr.version = ?)" clause when cv > 0, otherwise fall back
to the single "cr.version = ?" predicate; ensure the same cv variable is used
and that the cv-1 expression is only evaluated after checking cv > 0 so you
avoid unsigned underflow and invalid bound values being passed to the SQL
driver.
In `@internal/data/schema_inmemory.go`:
- Around line 298-317: The current branch treats any error from
loadResourceSchema as a missing file and logs "No schema found", which masks
real read failures; change the logic around loadResourceSchema(resourceLabel,
reporterLabel, resourceDir) so that if err != nil you return or propagate that
error (including context about resourceLabel/reporterLabel), only treat
isReporterSchemaExists == false as the benign "no schema" case that should log a
warning, and keep the existing successful path that constructs resRT/repRT and
calls repository.CreateReporterSchema with
validationSchemaFromString(reporterSchema).
---
Outside diff comments:
In `@internal/service/resources/kesselinventoryservice.go`:
- Around line 301-309: The paginationFromProto function currently swallows
errors from model.NewContinuationToken which can turn invalid continuation
tokens into a zero value; change paginationFromProto to return
(*model.Pagination, error), call model.NewContinuationToken and check its error,
and if parsing fails return nil and a descriptive error (e.g., "invalid
continuation token: ..."); update all callers of paginationFromProto to handle
the new error and propagate or convert it to the appropriate gRPC/HTTP error
response so invalid tokens aren’t silently accepted.
---
Nitpick comments:
In `@internal/biz/model/common.go`:
- Around line 102-119: The NewContinuationToken function currently returns an
unused error; if empty continuation tokens are valid, change
NewContinuationToken to return only (ContinuationToken) by removing the error
from its signature and implementation, update all call sites to accept a
single-return value, and adjust related helpers (e.g., any places that call
NewContinuationToken) while leaving ContinuationToken.String, Serialize, and
DeserializeContinuationToken unchanged; alternatively, if empty tokens must be
rejected, add validation in NewContinuationToken to return an error for empty
input and ensure callers handle the error.
In `@internal/data/fake_resource_repository.go`:
- Around line 256-311: The fake implementation of
FindCurrentAndPreviousVersionedRepresentations ignores operationType and always
tries to load the previous version; update
fakeResourceRepository.FindCurrentAndPreviousVersionedRepresentations to only
attempt the cv-1 lookup when operationType.OperationType() !=
bizmodel.OperationTypeCreated (and cv > 0). Concretely, wrap the block that
checks versionMap[cv-1] and constructs previous (the code using cv-1, v :=
bizmodel.NewVersion(...), and bizmodel.NewRepresentations(...)) in a conditional
that checks operationType.OperationType() and cv>0, matching the real repo
behavior.
🪄 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: 13d979ad-9f71-40ee-adad-6f60621b1e4e
📒 Files selected for processing (35)
internal/biz/model/common.gointernal/biz/model/lookup_objects_item.gointernal/biz/model/lookup_subjects_item.gointernal/biz/model/pagination.gointernal/biz/model/read_tuples_item.gointernal/biz/model/representations.gointernal/biz/model/resource_delete_event.gointernal/biz/model/resource_event.gointernal/biz/model/resource_report_event.gointernal/biz/model/resource_repository.gointernal/biz/model/schema_repository.gointernal/biz/model/schema_service.gointernal/biz/model/schema_service_test.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/biz/usecase/tuples/commands.gointernal/biz/usecase/tuples/tuple_crud_usecase.gointernal/biz/usecase/tuples/tuple_crud_usecase_test.gointernal/consumer/consumer.gointernal/consumer/consumer_test.gointernal/data/fake_resource_repository.gointernal/data/grpc_relations_repository.gointernal/data/grpc_relations_repository_test.gointernal/data/relations_simple.gointernal/data/resource_repository.gointernal/data/resource_repository_test.gointernal/data/schema_inmemory.gointernal/data/schema_inmemory_test.gointernal/service/resources/kesselinventoryservice.gointernal/service/resources/kesselinventoryservice_test.gointernal/service/resources/pagination_test.gointernal/service/tuples/tuples.gointernal/service/tuples/tuples_test.go
Comment on lines
+177
to
+182
| return uc.updateResource(tx, cmd, res, txid) | ||
| } | ||
|
|
||
| log.Info("Creating new resource") | ||
| operationType = model.OperationTypeCreated | ||
| return uc.createResource(tx, cmd, txid.String()) | ||
| return uc.createResource(tx, cmd, txid) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspecting the resource write path and outbox txid usage..."
fd 'resource_service.go$' internal -x sed -n '120,310p' {}
fd 'resource_repository.go$' internal -x sed -n '120,220p;360,430p' {}
fd 'outboxevents.go$' internal -x sed -n '200,250p' {}
echo
echo "Searching for the two transaction-id flows..."
rg -n 'HasTransactionIdBeenProcessed\(|model\.NewResource\(|existingResource\.Update\(|resourceRepository\.Save\(' internal -g '!**/*_test.go'Repository: project-kessel/inventory-api
Length of output: 19958
Resolve transaction ID inconsistency across the write path.
The resource model stores *cmd.TransactionId (lines 251, 283) while Save() receives and forwards a separate request-scoped txid to the outbox (lines 269, 296). When a caller supplies an idempotency key, these diverge from the start. The retry logic (lines 188–191) compounds this by rewriting cmd.TransactionId without updating the captured txid, causing the same operation to persist with different transaction IDs in the resource vs. outbox event stream. This breaks correlation between stored state and published events, compromising idempotency tracking and event replay.
Either thread a single effective transaction ID through both the resource model and outbox calls, or explicitly split these into distinct idempotency key and outbox correlation ID concepts with clear naming to prevent future regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/biz/usecase/resources/resource_service.go` around lines 177 - 182,
Save currently diverges between cmd.TransactionId and the local request-scoped
txid (used in createResource/updateResource and outbox), and the retry path
rewrites cmd.TransactionId without updating the captured txid; fix by computing
a single effective transaction ID up front (e.g. effectiveTxID := first non-nil
of cmd.TransactionId and request txid or generate one) and use that same
effectiveTxID everywhere: set cmd.TransactionId = effectiveTxID before any
retries, pass effectiveTxID into uc.createResource and uc.updateResource calls
and into the outbox invocation, or alternatively rename and separate concepts
explicitly (e.g. IdempotencyKey vs OutboxCorrelationId) and wire them
consistently so resource model (cmd.TransactionId), the local txid variable, and
outbox events always carry the same identifier.
Comment on lines
+313
to
+317
| cv := currentCommonVersion.Uint() | ||
| if operationType.OperationType() == bizmodel.OperationTypeCreated { | ||
| query = query.Where("cr.version = ?", *currentCommonVersion) | ||
| query = query.Where("cr.version = ?", cv) | ||
| } else { | ||
| query = query.Where("(cr.version = ? OR cr.version = ?)", *currentCommonVersion, *currentCommonVersion-1) | ||
|
|
||
| query = query.Where("(cr.version = ? OR cr.version = ?)", cv, cv-1) |
Contributor
There was a problem hiding this comment.
Avoid unsigned underflow in the previous-version query.
When currentCommonVersion is 0 on a non-create path, cv-1 wraps to math.MaxUint and gets bound into the SQL predicate. That can produce the wrong lookup, and some drivers reject that value for signed integer columns. Gate the previous-version clause behind cv > 0 so the query matches the guard you already use later in the loop.
Suggested fix
cv := currentCommonVersion.Uint()
- if operationType.OperationType() == bizmodel.OperationTypeCreated {
+ if operationType.OperationType() == bizmodel.OperationTypeCreated || cv == 0 {
query = query.Where("cr.version = ?", cv)
} else {
query = query.Where("(cr.version = ? OR cr.version = ?)", cv, cv-1)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cv := currentCommonVersion.Uint() | |
| if operationType.OperationType() == bizmodel.OperationTypeCreated { | |
| query = query.Where("cr.version = ?", *currentCommonVersion) | |
| query = query.Where("cr.version = ?", cv) | |
| } else { | |
| query = query.Where("(cr.version = ? OR cr.version = ?)", *currentCommonVersion, *currentCommonVersion-1) | |
| query = query.Where("(cr.version = ? OR cr.version = ?)", cv, cv-1) | |
| cv := currentCommonVersion.Uint() | |
| if operationType.OperationType() == bizmodel.OperationTypeCreated || cv == 0 { | |
| query = query.Where("cr.version = ?", cv) | |
| } else { | |
| query = query.Where("(cr.version = ? OR cr.version = ?)", cv, cv-1) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/data/resource_repository.go` around lines 313 - 317, The query
builds a previous-version predicate using cv-1 which underflows when
currentCommonVersion is 0; update the branch that sets query (around the use of
currentCommonVersion.Uint() and operationType.OperationType() ==
bizmodel.OperationTypeCreated) to only include the "(cr.version = ? OR
cr.version = ?)" clause when cv > 0, otherwise fall back to the single
"cr.version = ?" predicate; ensure the same cv variable is used and that the
cv-1 expression is only evaluated after checking cv > 0 so you avoid unsigned
underflow and invalid bound values being passed to the SQL driver.
Comment on lines
+298
to
+317
| reporterSchema, isReporterSchemaExists, err := loadResourceSchema(resourceLabel, reporterLabel, resourceDir) | ||
| if err == nil && isReporterSchemaExists { | ||
| resRT, err := bizmodel.NewResourceType(resourceLabel) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid resource type %q: %w", resourceLabel, err) | ||
| } | ||
| repRT, err := bizmodel.NewReporterType(reporterLabel) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid reporter type %q: %w", reporterLabel, err) | ||
| } | ||
| err = repository.CreateReporterSchema(ctx, bizmodel.ReporterSchema{ | ||
| ResourceType: resourceType, | ||
| ReporterType: reporterType, | ||
| ResourceType: resRT, | ||
| ReporterType: repRT, | ||
| ValidationSchema: validationSchemaFromString(reporterSchema), | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| log.Warnf("No schema found for %s:%s", resourceType, reporterType) | ||
| log.Warnf("No schema found for %s:%s", resourceLabel, reporterLabel) |
Contributor
There was a problem hiding this comment.
Return real reporter schema load failures instead of downgrading them to warnings.
loadResourceSchema only returns isReporterSchemaExists == false for a genuinely missing file. Any other error here means the reporter schema could not be read, but the current else path logs "No schema found" and continues with a partially loaded repository.
Suggested fix
reporterLabel := reporter.Name()
reporterSchema, isReporterSchemaExists, err := loadResourceSchema(resourceLabel, reporterLabel, resourceDir)
- if err == nil && isReporterSchemaExists {
+ if err != nil {
+ return nil, fmt.Errorf("failed to load reporter schema %s:%s: %w", resourceLabel, reporterLabel, err)
+ }
+ if isReporterSchemaExists {
resRT, err := bizmodel.NewResourceType(resourceLabel)
if err != nil {
return nil, fmt.Errorf("invalid resource type %q: %w", resourceLabel, err)
@@
if err != nil {
return nil, err
}
} else {
log.Warnf("No schema found for %s:%s", resourceLabel, reporterLabel)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| reporterSchema, isReporterSchemaExists, err := loadResourceSchema(resourceLabel, reporterLabel, resourceDir) | |
| if err == nil && isReporterSchemaExists { | |
| resRT, err := bizmodel.NewResourceType(resourceLabel) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid resource type %q: %w", resourceLabel, err) | |
| } | |
| repRT, err := bizmodel.NewReporterType(reporterLabel) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid reporter type %q: %w", reporterLabel, err) | |
| } | |
| err = repository.CreateReporterSchema(ctx, bizmodel.ReporterSchema{ | |
| ResourceType: resourceType, | |
| ReporterType: reporterType, | |
| ResourceType: resRT, | |
| ReporterType: repRT, | |
| ValidationSchema: validationSchemaFromString(reporterSchema), | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } else { | |
| log.Warnf("No schema found for %s:%s", resourceType, reporterType) | |
| log.Warnf("No schema found for %s:%s", resourceLabel, reporterLabel) | |
| reporterSchema, isReporterSchemaExists, err := loadResourceSchema(resourceLabel, reporterLabel, resourceDir) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to load reporter schema %s:%s: %w", resourceLabel, reporterLabel, err) | |
| } | |
| if isReporterSchemaExists { | |
| resRT, err := bizmodel.NewResourceType(resourceLabel) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid resource type %q: %w", resourceLabel, err) | |
| } | |
| repRT, err := bizmodel.NewReporterType(reporterLabel) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid reporter type %q: %w", reporterLabel, err) | |
| } | |
| err = repository.CreateReporterSchema(ctx, bizmodel.ReporterSchema{ | |
| ResourceType: resRT, | |
| ReporterType: repRT, | |
| ValidationSchema: validationSchemaFromString(reporterSchema), | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } 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 298 - 317, The current branch
treats any error from loadResourceSchema as a missing file and logs "No schema
found", which masks real read failures; change the logic around
loadResourceSchema(resourceLabel, reporterLabel, resourceDir) so that if err !=
nil you return or propagate that error (including context about
resourceLabel/reporterLabel), only treat isReporterSchemaExists == false as the
benign "no schema" case that should log a warning, and keep the existing
successful path that constructs resRT/repRT and calls
repository.CreateReporterSchema with validationSchemaFromString(reporterSchema).
Made-with: Cursor
snehagunta
force-pushed
the
RHCLOUD-45005-move-model-types-to-model
branch
from
67cfd19 to
8bc5ab6
Compare
Contributor
Author
|
Replaced by new PR on dedicated branch |
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
stringwithmodel.ResourceTypeandmodel.ReporterTypeinSchemaRepositoryinterface,ResourceSchema,ReporterSchema, and all implementationsResourceType.SchemaRepositoryKey()for canonical map lookups (trim, slash replacement, lowercase)ResourceEventinterface getters to returnResourceType/ReporterTypeinstead ofstring.String()at outbox serialization boundary inoutboxevents.goStack
4/4 in the tiny-types series. Depends on #1331 (merge that first).
Test plan
go build ./...passesgo test ./internal/...— all 36 packages passMade with Cursor
Summary by CodeRabbit