RHCLOUD-41760 - Initial pass on refactoring the optional fields to pointer#1224
Merged
snehagunta merged 30 commits intoproject-kessel:mainfrom Mar 31, 2026
Conversation
- Following fields - TransactionId - ConsoleHref - ReporterVersion (legacy) - ReporterRepresentation / CommonRepresentation - OptionsConfig
Collaborator
|
Can one of the admins verify this patch? |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors domain models to support optional fields by converting concrete types to pointers ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Rajagopalan-Ranganathan
changed the title
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/biz/model/resource_test.go (1)
262-270:⚠️ Potential issue | 🔴 Critical*Use TransactionId in Update calls to match the pointer-based API.
These Update invocations pass a
TransactionIdvalue. IfUpdatenow expects*TransactionId(as used elsewhere in this PR), these calls won’t compile and also bypass the “nil means unset” semantics. Wrap the value in a pointer (or use a small helper) consistently.🔧 Example fix (apply same pattern to other Update calls)
- err = original.Update( - nonExistentKey, - apiHref, - consoleHref, - nil, - commonData, - reporterData, - TransactionId("tx-nonexistent"), - ) + txID := TransactionId("tx-nonexistent") + err = original.Update( + nonExistentKey, + apiHref, + consoleHref, + nil, + commonData, + reporterData, + &txID, + )#!/bin/bash # Verify Update signature and call sites rg -n "func \\(.*\\) Update\\(" internal/biz/model rg -n "Update\\(" internal/biz/model/resource_test.goAlso applies to: 297-305, 340-348, 398-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/biz/model/resource_test.go` around lines 262 - 270, The Update calls on the resource test pass a TransactionId value but Update expects a *TransactionId; change each call (e.g., original.Update(..., TransactionId("tx-nonexistent")) and the other occurrences around lines noted) to pass a pointer instead (e.g., &TransactionId("tx-nonexistent")) or use a small helper like NewTransactionIdPtr("tx-...") to produce *TransactionId; update all matching calls (including the ones at the other ranges) so nil semantics remain respected and the code compiles with the pointer-based Update signature.internal/biz/model/reporter_resource.go (1)
193-215:⚠️ Potential issue | 🟠 MajorRemove leftover debug
log.Printfstatements before merging.Lines 195–196 dump the full
ReporterResourceSnapshotto stdout on every deserialization call. This is a debug artifact that:
- Pollutes logs in production with verbose output on every resource lookup.
- Could inadvertently log sensitive resource data.
- Uses stdlib
loginstead of the structured Kratos logger used elsewhere in the codebase.Proposed fix
func DeserializeReporterResource(snapshot ReporterResourceSnapshot) ReporterResource { - log.Printf("----------------------------------") - log.Printf("ReporterResourceSnapshot : %+v, ", snapshot) - var consoleHref *ConsoleHref🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/biz/model/reporter_resource.go` around lines 193 - 215, In DeserializeReporterResource remove the two debug log.Printf calls that print the entire ReporterResourceSnapshot (the stdlib log.Printf lines near the top of the function) so you don't leak verbose/sensitive data or mix logging systems; if you need a runtime trace instead, replace them with a single structured Kratos logger call (using the package's logger) with minimal non-sensitive fields, otherwise simply delete the log.Printf statements from DeserializeReporterResource.
🧹 Nitpick comments (4)
internal/service/resources/kesselinventoryservice_test.go (1)
3080-3080: Both nil-representation error assertions use the same generic substring.
"representation required"at lines 3080 and 3093 now covers both "nil reporter (common-only)" and "nil common (reporter-only)" cases with the same substring. If the service ever swaps the error messages for the two fields, these tests would still pass. Using the field-specific prefix (e.g."invalid ReporterRepresentation"vs"invalid CommonRepresentation") — as done in the sibling testTestInventoryService_ReportResource_NilOrEmptyRepresentationStructsat lines 2869 and 2880 — would give stronger guarantees.Also applies to: 3093-3093
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/resources/kesselinventoryservice_test.go` at line 3080, The test assertions use the same generic expectMsgContains "representation required" for both nil-representation cases; update each assertion to use the field-specific error substring used elsewhere so the tests assert the correct field-specific error: change the expectMsgContains for the reporter-related case to "invalid ReporterRepresentation" and change the expectMsgContains for the common-related case to "invalid CommonRepresentation" (these appear in the tests around TestInventoryService_ReportResource_NilOrEmptyRepresentationStructs and the two failing cases at lines ~3080/3093) so each test validates the correct field-specific error.internal/biz/model/errors.go (1)
15-15: Consider adding aValidateNotNilhelper for consistency.Every other sentinel in this block has a corresponding helper (
ValidateRequired,ValidateStringRequired, etc.).ErrNilis the only one without one, which means callers must manually wrap it.♻️ Suggested addition
+// ValidateNotNil validates that a pointer is not nil +func ValidateNotNil(field string, value any) error { + if value == nil { + return fmt.Errorf("%w: %s", ErrNil, field) + } + return nil +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/biz/model/errors.go` at line 15, Add a ValidateNotNil helper to mirror the other validators so callers don't manually wrap ErrNil; implement a function named ValidateNotNil(value interface{}, field string) error that returns nil when value != nil and otherwise returns a wrapped ErrNil (e.g., fmt.Errorf("%s: %w", field, ErrNil)) so it matches the style of ValidateRequired and ValidateStringRequired and can be used by callers of ErrNil.internal/data/resource_repository_test.go (1)
1965-1966: Silently discarding error fromNewApiHref("")in test helpers.
NewApiHref("")returns an error (per theNewApiHrefconstructor which validates non-empty URI), but the error is discarded with_. This yields a zero-valueApiHref("")which happens to work here, but it's fragile — if validation behavior changes, these tests will silently pass bad data.Consider using a known-valid URL or explicitly asserting the error:
Proposed fix
- emptyApiHref, _ := bizmodel.NewApiHref("") - err = resource.Update(key, emptyApiHref, nil, nil, &updatedReporter1, &updatedCommon1, &transactionId1) + apiHref, err := bizmodel.NewApiHref("https://api.example.com/placeholder") + require.NoError(t, err) + err = resource.Update(key, apiHref, nil, nil, &updatedReporter1, &updatedCommon1, &transactionId1)Also applies to: 2060-2061
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/data/resource_repository_test.go` around lines 1965 - 1966, The test is discarding the error returned by NewApiHref("") which hides validation failures; update the test to call NewApiHref with a known-valid non-empty URL (e.g., "https://example.com") or capture and assert the error is nil before using the value, then pass the resulting ApiHref into resource.Update (the call that currently uses emptyApiHref). Ensure you replace both occurrences noted (around NewApiHref("") at the Update(...) calls) so tests use a validated ApiHref or explicitly fail if NewApiHref returns an error.internal/biz/model/resource.go (1)
100-156: Duplicated nil-dereferencing logic betweenNewResourceandUpdate.The nil-handling for
transactionId,reporterRepresentationData,commonRepresentationData, andconsoleHref(lines 119–140) is nearly identical toNewResource(lines 26–63). Consider extracting a small helper to reduce duplication, e.g.:Example helper
func resolveOptionalFields( transactionId *TransactionId, consoleHref *ConsoleHref, reporterRepresentationData *Representation, commonRepresentationData *Representation, ) (TransactionId, ConsoleHref, Representation, Representation, error) { var txId TransactionId if transactionId == nil || *transactionId == "" { var err error txId, err = GenerateTransactionId() if err != nil { return txId, ConsoleHref(""), Representation{}, Representation{}, err } } else { txId = *transactionId } var reporterData, commonData Representation if reporterRepresentationData != nil { reporterData = *reporterRepresentationData } if commonRepresentationData != nil { commonData = *commonRepresentationData } var ch ConsoleHref if consoleHref != nil { ch = *consoleHref } return txId, ch, reporterData, commonData, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/biz/model/resource.go` around lines 100 - 156, The Update method duplicates nil-handling logic found in NewResource for transactionId, consoleHref, reporterRepresentationData, and commonRepresentationData; extract that logic into a small helper (e.g., resolveOptionalFields) and call it from both NewResource and Update to return (TransactionId, ConsoleHref, Representation, Representation, error), preserving the current behavior including calling GenerateTransactionId when transactionId is nil/empty and propagating any GenerateTransactionId error; update Update to use resolveOptionalFields and remove the inlined nil checks and copying of pointer values.
🤖 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/model_legacy/outboxevents_test.go`:
- Around line 157-175: The test comment in
TestEventResourceReporter_ReporterVersion_Optional is inaccurate:
EventResourceReporter.ReporterVersion is not tagged with `omitempty`, so a nil
ReporterVersion serializes as the field present with value null, not as
omitted/zero; update the comment to state "Nil ReporterVersion round-trips as
JSON null (field present)" or alternatively add `omitempty` to
EventResourceReporter.ReporterVersion if you intend the field to be omitted when
nil—refer to EventResourceReporter and the ReporterVersion field when making the
change.
In `@internal/biz/model_legacy/outboxevents.go`:
- Around line 99-105: EventResourceReporter.ReporterVersion currently is a
*string without `omitempty`, causing nil to serialize as null while
EventRelationshipReporter.ReporterVersion uses `omitempty`; update the
EventResourceReporter struct by adding `omitempty` to the ReporterVersion tag so
both structs serialize absent/nil the same way (match
EventRelationshipReporter.ReporterVersion) — locate the EventResourceReporter
type and change the `ReporterVersion` json tag to include `omitempty`.
In `@internal/biz/model/reporter_representation.go`:
- Around line 40-45: Reinstate validation to reject empty byte representations:
in the constructor/validator that checks reporterResourceID.UUID() and data (the
code handling ReporterDataRepresentation), after the nil check add a check for
len(data) == 0 and return fmt.Errorf("%w: Representation (data)", ErrNil) for
empty slices so empty representations don't pass; if your model supports
tombstones, only skip the length check for tombstone representations (follow the
same behavior as NewCommonRepresentation).
In `@internal/config/config.go`:
- Around line 150-158: The docstring is wrong and the function leaves a
partially-configured state by always setting o.Authz.Authz = authz.Kessel even
when o.Authz.Kessel == nil; update ConfigureAuthz so it returns early if o ==
nil || o.Authz == nil || o.Authz.Kessel == nil (or alternatively only set
o.Authz.Authz and o.Authz.Kessel.URL inside a check that o.Authz.Kessel != nil),
and update the comment to reflect the real behavior (no-op if Authz or Kessel
options are nil); reference: OptionsConfig.ConfigureAuthz, o.Authz,
o.Authz.Kessel.
In `@internal/service/resources/kesselinventoryservice_test.go`:
- Line 2850: The table-driven test
TestInventoryService_ReportResource_NilOrEmptyRepresentationStructs contains a
dead field expectSuccess (and unused branch requireSuccess()) because no case
sets expectSuccess to true; either remove the expectSuccess field and the
requireSuccess() branch and call requireErrorContaining directly for each case,
or restore the missing positive test case (e.g., add a case with both non-nil,
non-empty representation structs and expectSuccess=true) so requireSuccess() is
reachable; update the test table and the test loop accordingly to keep
consistency between expectSuccess and calls to
requireSuccess()/requireErrorContaining.
- Line 3048: In TestInventoryService_ReportResource_ValidationErrorFormats the
expectSuccess boolean is dead code because no cases set it true; update the
tests by either removing the expectSuccess field and directly calling
requireErrorContaining(...) in each case (eliminate the unreachable
requireSuccess() branch), or add a test case that sets expectSuccess: true and
keeps the requireSuccess() path; apply the same change for the similar
occurrence around the second block (the other table-driven cases at the
referenced spot) so that only reachable expectation branches
(requireErrorContaining or requireSuccess) remain.
- Around line 2836-2838: The file comment above the representation validation
tests is incorrect: update the comment near the tests in
kesselinventoryservice_test.go to state that empty structs are rejected rather
than accepted; specifically, amend the block comment that currently says "Empty
structs are accepted by the domain (empty representation data)" to reflect that
empty representation structs are not allowed and produce codes.InvalidArgument
(see the test case "both empty structs" in TestCreateRepresentation / related
test functions to mirror the expected error "representation data cannot be
empty").
---
Outside diff comments:
In `@internal/biz/model/reporter_resource.go`:
- Around line 193-215: In DeserializeReporterResource remove the two debug
log.Printf calls that print the entire ReporterResourceSnapshot (the stdlib
log.Printf lines near the top of the function) so you don't leak
verbose/sensitive data or mix logging systems; if you need a runtime trace
instead, replace them with a single structured Kratos logger call (using the
package's logger) with minimal non-sensitive fields, otherwise simply delete the
log.Printf statements from DeserializeReporterResource.
In `@internal/biz/model/resource_test.go`:
- Around line 262-270: The Update calls on the resource test pass a
TransactionId value but Update expects a *TransactionId; change each call (e.g.,
original.Update(..., TransactionId("tx-nonexistent")) and the other occurrences
around lines noted) to pass a pointer instead (e.g.,
&TransactionId("tx-nonexistent")) or use a small helper like
NewTransactionIdPtr("tx-...") to produce *TransactionId; update all matching
calls (including the ones at the other ranges) so nil semantics remain respected
and the code compiles with the pointer-based Update signature.
---
Nitpick comments:
In `@internal/biz/model/errors.go`:
- Line 15: Add a ValidateNotNil helper to mirror the other validators so callers
don't manually wrap ErrNil; implement a function named ValidateNotNil(value
interface{}, field string) error that returns nil when value != nil and
otherwise returns a wrapped ErrNil (e.g., fmt.Errorf("%s: %w", field, ErrNil))
so it matches the style of ValidateRequired and ValidateStringRequired and can
be used by callers of ErrNil.
In `@internal/biz/model/resource.go`:
- Around line 100-156: The Update method duplicates nil-handling logic found in
NewResource for transactionId, consoleHref, reporterRepresentationData, and
commonRepresentationData; extract that logic into a small helper (e.g.,
resolveOptionalFields) and call it from both NewResource and Update to return
(TransactionId, ConsoleHref, Representation, Representation, error), preserving
the current behavior including calling GenerateTransactionId when transactionId
is nil/empty and propagating any GenerateTransactionId error; update Update to
use resolveOptionalFields and remove the inlined nil checks and copying of
pointer values.
In `@internal/data/resource_repository_test.go`:
- Around line 1965-1966: The test is discarding the error returned by
NewApiHref("") which hides validation failures; update the test to call
NewApiHref with a known-valid non-empty URL (e.g., "https://example.com") or
capture and assert the error is nil before using the value, then pass the
resulting ApiHref into resource.Update (the call that currently uses
emptyApiHref). Ensure you replace both occurrences noted (around NewApiHref("")
at the Update(...) calls) so tests use a validated ApiHref or explicitly fail if
NewApiHref returns an error.
In `@internal/service/resources/kesselinventoryservice_test.go`:
- Line 3080: The test assertions use the same generic expectMsgContains
"representation required" for both nil-representation cases; update each
assertion to use the field-specific error substring used elsewhere so the tests
assert the correct field-specific error: change the expectMsgContains for the
reporter-related case to "invalid ReporterRepresentation" and change the
expectMsgContains for the common-related case to "invalid CommonRepresentation"
(these appear in the tests around
TestInventoryService_ReportResource_NilOrEmptyRepresentationStructs and the two
failing cases at lines ~3080/3093) so each test validates the correct
field-specific error.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (28)
docs/optional-fields-inconsistent-handling.mddocs/optional-fields-inventory.mddocs/plan-optional-fields-refactor.mdinternal/biz/model/common_representation.gointernal/biz/model/errors.gointernal/biz/model/reporter_representation.gointernal/biz/model/reporter_representation_test.gointernal/biz/model/reporter_resource.gointernal/biz/model/reporter_resource_test.gointernal/biz/model/resource.gointernal/biz/model/resource_test.gointernal/biz/model/snapshots.gointernal/biz/model/snapshots_test.gointernal/biz/model/testdata.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/config/config.gointernal/config/config_test.gointernal/consumer/consumer_test.gointernal/data/fake_resource_repository.gointernal/data/model/common_representation.gointernal/data/model/reporter_representation.gointernal/data/model/reporter_resource.gointernal/data/resource_repository.gointernal/data/resource_repository_test.gointernal/service/resources/kesselinventoryservice_test.go
address coderabbit comments
snehagunta
reviewed
Feb 24, 2026
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/config/config.go (1)
57-112:⚠️ Potential issue | 🟠 MajorAdd nil guards for nested pointer fields in LogConfigurationInfo.
Lines 60–61 dereference
options.Server.HttpOptionsandoptions.Server.GrpcOptionswithout checking if they are nil. Similarly, lines 89–95 dereferenceoptions.Storage.Postgresand lines 102–111 dereferenceoptions.Consumer.RetryOptionsandoptions.Consumer.AuthOptionswithout nil checks. While the normal code path (viaNewOptionsConfig()) initializes these nested pointers, the function should defend against edge cases where an option group exists but its nested fields remain nil. Add nil guards before dereferencing these nested pointer fields, or document the guarantee that they are always initialized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 57 - 112, The LogConfigurationInfo routine dereferences nested pointer fields without nil guards; add checks before accessing options.Server.HttpOptions and options.Server.GrpcOptions, options.Storage.Postgres, options.Consumer.RetryOptions, and options.Consumer.AuthOptions (or use local safe-default vars) so you only read their fields when the nested pointer != nil, and fall back to empty/default values in the log when they are nil; update the blocks that log Server, Storage (Postgres) and Consumer configuration in LogConfigurationInfo to guard these nested pointers (e.g., check options.Server != nil && options.Server.HttpOptions != nil, etc.) before using their Addr/Host/Port/RetryOptions/AuthOptions fields.
♻️ Duplicate comments (1)
internal/biz/model_legacy/outboxevents_test.go (1)
157-160:⚠️ Potential issue | 🟡 MinorComment on line 160 is self-contradictory (still inaccurate after prior fix attempt).
Line 160 reads:
// Nil ReporterVersion round-trips as null in JSON (omitempty omits the field when nil)The first clause ("as null in JSON") says the field is serialized as
null, but the parenthetical correctly explains thatomitemptycauses the field to be omitted entirely. Both cannot be true. SinceReporterVersionis*stringwithomitempty, a nil value causes the key to be absent from the JSON output — not set tonull.✏️ Proposed fix
-// Nil ReporterVersion round-trips as null in JSON (omitempty omits the field when nil) +// Nil ReporterVersion: omitempty causes the field to be absent from JSON when nil, +// and unmarshal of that JSON leaves ReporterVersion nil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/biz/model_legacy/outboxevents_test.go` around lines 157 - 160, The comment in TestEventResourceReporter_ReporterVersion_Optional is contradictory: update the comment that currently states "Nil ReporterVersion round-trips as null in JSON (omitempty omits the field when nil)" to correctly state that a nil *string ReporterVersion is omitted from the marshaled JSON (the key is absent) because of the `omitempty` tag; reference the TestEventResourceReporter_ReporterVersion_Optional test name and the ReporterVersion field when making this wording change.
🧹 Nitpick comments (2)
internal/data/resource_repository_test.go (1)
1605-1884: No test covers theniltransactionId auto-generation path inNewResource.All helper functions pass
&transactionId(non-nil). TheNewResourcedocstring explicitly states that passingnilcauses it to auto-generate a newTransactionId, but this behavior is untested. A simple variant in one of the helpers (or a dedicated test) would close this coverage gap for a newly introduced pointer-based API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/data/resource_repository_test.go` around lines 1605 - 1884, Test coverage is missing for the NewResource(nil) auto-generation of TransactionId; add a new unit helper/test that calls bizmodel.NewResource with a nil transactionId (e.g., create a new helper similar to createTestResourceWithReporter or createTestResourceWithReporterDataOnly but pass nil instead of &transactionId) and assert the returned Resource contains a non-nil/valid TransactionId (and that it differs from zero values or matches NewTransactionId format), using the same validation helpers (require.NoError, etc.) to ensure the auto-generated transaction id path in NewResource is exercised.internal/biz/model/resource_test.go (1)
15-18: Remove the unusedtimeValfunction (lines 15–18).This helper is declared but never called anywhere in the codebase. The function body is a no-op identity wrapper, so it can be safely removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/biz/model/resource_test.go` around lines 15 - 18, Remove the unused identity helper function timeVal by deleting its declaration (func timeVal(t time.Time) time.Time { return t }) from the test file; ensure no references remain and run go test / go vet to confirm the codebase still builds cleanly after the function is removed.
🤖 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/model_legacy/outboxevents_test.go`:
- Around line 157-158: Update the stale doc comment in
TestEventResourceReporter_ReporterVersion_Optional: remove the forward-reference
"Phase 2 will align EventRelationshipReporter" since
TestEventRelationshipReporter_ReporterVersion_Optional already implements that
alignment in this PR; leave the rest of the comment describing that
EventResourceReporter uses *string for ReporterVersion (nil = not set) so the
intent remains clear.
In `@internal/biz/model/errors.go`:
- Around line 76-81: ValidateNotNil currently only checks value == nil which
misses typed nil pointers; update ValidateNotNil to detect underlying nils
(e.g., using reflect.ValueOf(value).Kind()==reflect.Ptr &&
reflect.ValueOf(value).IsNil()) and return ErrNil when the underlying pointer is
nil, or replace the signature with a generic pointer version (ValidateNotNil[T
any](field string, value *T) error) so callers passing typed nil pointers are
correctly treated as nil; ensure the function still formats the error with
ErrNil and the field name as before.
In `@internal/biz/model/resource_test.go`:
- Around line 445-447: NewTransactionId returns a single TransactionId, but the
test code is unpacking two values; update each occurrence where you do
emptyTxID, _ := NewTransactionId("") to use a single-value assignment emptyTxID
:= NewTransactionId("") (e.g., the uses around the Update call that passes
&emptyTxID and other places in the same file), ensuring all references to
NewTransactionId in resource_test.go use the single return value.
---
Outside diff comments:
In `@internal/config/config.go`:
- Around line 57-112: The LogConfigurationInfo routine dereferences nested
pointer fields without nil guards; add checks before accessing
options.Server.HttpOptions and options.Server.GrpcOptions,
options.Storage.Postgres, options.Consumer.RetryOptions, and
options.Consumer.AuthOptions (or use local safe-default vars) so you only read
their fields when the nested pointer != nil, and fall back to empty/default
values in the log when they are nil; update the blocks that log Server, Storage
(Postgres) and Consumer configuration in LogConfigurationInfo to guard these
nested pointers (e.g., check options.Server != nil && options.Server.HttpOptions
!= nil, etc.) before using their Addr/Host/Port/RetryOptions/AuthOptions fields.
---
Duplicate comments:
In `@internal/biz/model_legacy/outboxevents_test.go`:
- Around line 157-160: The comment in
TestEventResourceReporter_ReporterVersion_Optional is contradictory: update the
comment that currently states "Nil ReporterVersion round-trips as null in JSON
(omitempty omits the field when nil)" to correctly state that a nil *string
ReporterVersion is omitted from the marshaled JSON (the key is absent) because
of the `omitempty` tag; reference the
TestEventResourceReporter_ReporterVersion_Optional test name and the
ReporterVersion field when making this wording change.
---
Nitpick comments:
In `@internal/biz/model/resource_test.go`:
- Around line 15-18: Remove the unused identity helper function timeVal by
deleting its declaration (func timeVal(t time.Time) time.Time { return t }) from
the test file; ensure no references remain and run go test / go vet to confirm
the codebase still builds cleanly after the function is removed.
In `@internal/data/resource_repository_test.go`:
- Around line 1605-1884: Test coverage is missing for the NewResource(nil)
auto-generation of TransactionId; add a new unit helper/test that calls
bizmodel.NewResource with a nil transactionId (e.g., create a new helper similar
to createTestResourceWithReporter or createTestResourceWithReporterDataOnly but
pass nil instead of &transactionId) and assert the returned Resource contains a
non-nil/valid TransactionId (and that it differs from zero values or matches
NewTransactionId format), using the same validation helpers (require.NoError,
etc.) to ensure the auto-generated transaction id path in NewResource is
exercised.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
internal/biz/model/errors.gointernal/biz/model/resource_test.gointernal/biz/model_legacy/outboxevents.gointernal/biz/model_legacy/outboxevents_test.gointernal/config/config.gointernal/data/resource_repository_test.gointernal/service/resources/kesselinventoryservice_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/biz/model_legacy/outboxevents.go
11 tasks
Contributor
|
Hey @Rajagopalan-Ranganathan the big one that stands out to me is
|
Contributor
|
I am also wondering if config files need to be updated at all. I have to look more closely to confirm |
Contributor
|
full_cross-layer_field_analysis_18ef4719.plan.md I am attaching an analysis that I got from cursor that makes sense to me, this has the full list of fields where there is a mismatch. The prompt I used, in case you were interested "I want to refactor to use optionals consistently across the codebase. Look for options where something is optional in the proto but not modeled as such in other layers. an exception is transactionId where it is optional in the proto, but required in the domain and db. But transactioId should be generated pretty early in the service layer if not provided in the input. show me a table across layers for fields that should be optional but are not modeled as such" This did not get me all the fields I was expecting, so I did "can you do the same analysis for all the fields across all apis" This got representations incorrectly, so I did this "representations are similar, but slightly different. Both are optional, but atleast one needs to be provided. also if it is required or not is determined by schema which is validated in the usecase." Hope this helps. |
…e service; make consoleHref optional in all layers; make representations optional in all layers and enforce that atleast one should be populated
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/biz/model/resource.go (1)
201-244:⚠️ Potential issue | 🟠 MajorNil representation inputs currently create an invalid “latest” state.
When either side is omitted, this helper just leaves that representation nil. On
Update, the new event becomes the only current report event, andSerialize()then emits a nil snapshot for the omitted side. That means a reporter-only/common-only update can silently drop the other representation from the aggregate’s latest state. The both-nil case also succeeds here even though the PR requires at least one representation to be populated.Please either carry the previous representation/version forward for omitted inputs or fail fast here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/biz/model/resource.go` around lines 201 - 244, The constructor path currently allows reporterRepresentation or commonRepresentation to be nil which leads NewResourceReportEvent/Serialize to produce an invalid “latest” state; update the code around NewReporterDataRepresentation/NewCommonRepresentation so that if reporterData or commonData is nil you either (a) look up and clone the prior/latest ReporterDataRepresentation/CommonRepresentation and their version values and pass those into NewResourceReportEvent, or (b) return an explicit error when both representations are nil (and when one is nil but no prior state exists), ensuring NewResourceReportEvent always receives at least one non-nil representation and correct version info rather than dropping previous state.
🤖 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/model/resource.go`:
- Around line 25-35: The NewResource constructor currently mints a TransactionId
when the caller passes nil/empty (using GenerateTransactionId), which breaks
idempotency; remove that generation and make transactionId required by the
domain: in NewResource (and any other constructors using the same pattern) stop
calling GenerateTransactionId, validate that transactionId is non-nil and
non-empty, and return an error if it's missing; simply assign txId =
*transactionId when valid so TransactionId is always supplied by the service
layer.
---
Outside diff comments:
In `@internal/biz/model/resource.go`:
- Around line 201-244: The constructor path currently allows
reporterRepresentation or commonRepresentation to be nil which leads
NewResourceReportEvent/Serialize to produce an invalid “latest” state; update
the code around NewReporterDataRepresentation/NewCommonRepresentation so that if
reporterData or commonData is nil you either (a) look up and clone the
prior/latest ReporterDataRepresentation/CommonRepresentation and their version
values and pass those into NewResourceReportEvent, or (b) return an explicit
error when both representations are nil (and when one is nil but no prior state
exists), ensuring NewResourceReportEvent always receives at least one non-nil
representation and correct version info rather than dropping previous state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f849dbf-286e-41f1-b1e4-4892c8b34c6c
📒 Files selected for processing (15)
cmd/jobs/resource_delete_job_test.gointernal/biz/model/common_representation.gointernal/biz/model/reporter_representation.gointernal/biz/model/resource.gointernal/biz/model/resource_report_event.gointernal/biz/model/snapshots.gointernal/biz/model/snapshots_test.gointernal/biz/model_legacy/outboxevents_test.gointernal/data/fake_resource_repository.gointernal/data/model/common_representation.gointernal/data/model/reporter_resource.gointernal/data/model/reporter_resource_test.gointernal/data/model/testdata.gointernal/data/resource_repository.gointernal/data/resource_repository_test.go
💤 Files with no reviewable changes (1)
- internal/data/model/common_representation.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/biz/model/common_representation.go
- internal/biz/model/reporter_representation.go
- internal/biz/model_legacy/outboxevents_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/biz/model/resource.go (2)
84-124:⚠️ Potential issue | 🟠 MajorValidate/build before mutating the aggregate.
Line 93 increments
commonVersion, and Line 100 updatesreporterResource, before the newtransactionIdcheck on Line 102 and beforeresourceEventAndRepresentations(...)can fail. That means a bad transaction ID or invalid representation can return an error afterResourcehas already been partially mutated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/biz/model/resource.go` around lines 84 - 124, The Update method mutates the aggregate (r.commonVersion and reporterResource via reporterResource.Update) before validating transactionId and before calling resourceEventAndRepresentations which can fail; move validation and construction of resourceEvent first: check transactionId (nil/empty) up-front, then call resourceEventAndRepresentations using the existing reporterResource fields (resourceID, representationVersion, generation, Id()) to build/validate the event, and only after that increment r.commonVersion and call reporterResource.Update and any other mutations; keep the same error returns but ensure no state changes happen before the event is successfully created.
189-232:⚠️ Potential issue | 🔴 CriticalAdd validation to reject when both representations are nil.
The helper currently allows
reporterData == nilandcommonData == nil, then forwards both as nil toNewResourceReportEvent(...). While upstream validation in the service layer requires at least one representation, the domain model layer (NewResource,Update,resourceEventAndRepresentations, andNewResourceReportEvent) does not enforce this invariant. Add a validation check to reject the both-nil case in the helper or constructor to ensure the invariant is maintained at the domain model level, preventing empty report events even if called outside the service layer context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/biz/model/resource.go` around lines 189 - 232, The code currently allows creating a ResourceReportEvent with both reporterRepresentation and commonRepresentation nil; add a validation that returns an error when both are nil (either in this helper before calling NewResourceReportEvent or inside NewResourceReportEvent / resourceEventAndRepresentations) to enforce the invariant required by the domain model; specifically check the pointers reporterRepresentation and commonRepresentation and return a descriptive error (e.g., "at least one representation required") from this function (or have NewResourceReportEvent return that error) so NewResource, Update, resourceEventAndRepresentations and NewResourceReportEvent never produce empty report events.
🤖 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/model/reporter_resource.go`:
- Around line 193-200: The function DeserializeReporterResource is currently
dumping the entire ReporterResourceSnapshot via log.Printf (the two log lines at
the top of DeserializeReporterResource), which leaks metadata; remove those
log.Printf calls so the snapshot is not emitted on the normal load path. Locate
DeserializeReporterResource and delete the lines that call
log.Printf("----------------------------------") and
log.Printf("ReporterResourceSnapshot : %+v, ", snapshot); if you still need
diagnostics consider using a debug-level logger behind a flag rather than
printing the snapshot directly.
In `@internal/biz/model/resource_test.go`:
- Around line 339-347: The test is passing a non-nil pointer (&consoleHref) so
it doesn't exercise the "omitted consoleHref" path; in the call to
original.Update (with TransactionId("tx-empty-console-href"),
reporterResourceKey, apiHref, ... reporterData, commonData, txID) replace the
&consoleHref argument with nil so the Update invocation models an absent
consoleHref and triggers the omitted-field behavior in
ReporterResource.Serialize.
In `@internal/biz/model/snapshots_test.go`:
- Around line 238-255: The test is incorrectly "blessing" an empty TransactionId
on snapshot structs; update
TestCommonRepresentationSnapshot_TransactionId_EmptyMeansNotSet (and the similar
ReporterRepresentationSnapshot test at 257-275) so they do not set TransactionId
to the empty string on
CommonRepresentationSnapshot/ReporterRepresentationSnapshot; instead provide a
valid non-empty TransactionId (e.g., a UUID string) or omit the field so the
domain generator simulates missing IDs, and adjust the assertion to expect the
provided non-empty TransactionId to round-trip (or to assert that an empty input
is treated as invalid rather than silently preserved).
In `@internal/biz/usecase/resources/resource_service.go`:
- Around line 563-572: The current early-return checks that reject when
cmd.ReporterRepresentation or cmd.CommonRepresentation are nil (returning
RepresentationRequiredError) are too strict; remove these conditional branches
so this service no longer enforces presence of either representation and instead
lets schema validation (per guidance and implementations in
internal/service/resources/kesselinventoryservice.go) decide required fields.
Locate the block referencing cmd.ReporterRepresentation,
cmd.CommonRepresentation and RepresentationRequiredError in resource_service.go
and delete the three if-return checks so the flow continues to schema validation
rather than failing early.
---
Outside diff comments:
In `@internal/biz/model/resource.go`:
- Around line 84-124: The Update method mutates the aggregate (r.commonVersion
and reporterResource via reporterResource.Update) before validating
transactionId and before calling resourceEventAndRepresentations which can fail;
move validation and construction of resourceEvent first: check transactionId
(nil/empty) up-front, then call resourceEventAndRepresentations using the
existing reporterResource fields (resourceID, representationVersion, generation,
Id()) to build/validate the event, and only after that increment r.commonVersion
and call reporterResource.Update and any other mutations; keep the same error
returns but ensure no state changes happen before the event is successfully
created.
- Around line 189-232: The code currently allows creating a ResourceReportEvent
with both reporterRepresentation and commonRepresentation nil; add a validation
that returns an error when both are nil (either in this helper before calling
NewResourceReportEvent or inside NewResourceReportEvent /
resourceEventAndRepresentations) to enforce the invariant required by the domain
model; specifically check the pointers reporterRepresentation and
commonRepresentation and return a descriptive error (e.g., "at least one
representation required") from this function (or have NewResourceReportEvent
return that error) so NewResource, Update, resourceEventAndRepresentations and
NewResourceReportEvent never produce empty report events.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 889c138a-3cb2-451c-9592-12fa9951c3ed
📒 Files selected for processing (10)
internal/biz/model/reporter_resource.gointernal/biz/model/reporter_resource_test.gointernal/biz/model/resource.gointernal/biz/model/resource_report_event.gointernal/biz/model/resource_test.gointernal/biz/model/snapshots.gointernal/biz/model/snapshots_test.gointernal/biz/model/testdata.gointernal/biz/model_legacy/outboxevents_test.gointernal/biz/usecase/resources/resource_service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/biz/model/resource_report_event.go
snehagunta
reviewed
Mar 6, 2026
snehagunta
reviewed
Mar 6, 2026
snehagunta
reviewed
Mar 6, 2026
snehagunta
reviewed
Mar 6, 2026
snehagunta
reviewed
Mar 6, 2026
snehagunta
reviewed
Mar 6, 2026
…stent inoutbox events
snehagunta
reviewed
Mar 6, 2026
snehagunta
reviewed
Mar 24, 2026
snehagunta
reviewed
Mar 24, 2026
Contributor
Author
|
/lgtm |
Contributor
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
3 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.
PR Template:
Describe your changes
Ticket reference (if applicable)
Fixes #
Checklist
Are the agreed upon acceptance criteria fulfilled?
Was the 4-eye-principle applied? (async PR review, pairing, ensembling)
Do your changes have passing automated tests and sufficient observability?
Are the work steps you introduced repeatable by others, either through automation or documentation?
The Changes were automatically built, tested, and - if needed, behind a feature flag - deployed to our production environment. (Please check this when the new deployment is done and you could verify it.)
Are the agreed upon coding/architectural practices applied?
Are security needs fullfilled? (e.g. no internal URL)
Is the corresponding Ticket in the right state? (should be on "review" now, put to done when this change made it to production)
For changes to the public API / code dependencies: Was the whole team (or a sufficient amount of ppl) able to review?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor