feat: allow positional parameter matching#2795
Open
mezakos wants to merge 1 commit intoPermify:masterfrom
Open
feat: allow positional parameter matching#2795mezakos wants to merge 1 commit intoPermify:masterfrom
mezakos wants to merge 1 commit intoPermify:masterfrom
Conversation
📝 WalkthroughWalkthroughThis PR refactors rule argument representation from an unordered map-based structure to an ordered slice-based structure using NamedArgument objects, updating proto definitions, AST structures, compilers, engines, and all dependent code. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
mezakos
changed the title
mezakos
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api-reference/openapiv2/apidocs.swagger.json (1)
2970-2983:⚠️ Potential issue | 🟠 MajorBreaking API change:
RuleDefinition.argumentschanged from map to array.This is a backwards-incompatible change to the public API. Previously
argumentswas an object (map of string→AttributeType), now it's an array ofNamedArgument. Existing clients parsing schema read responses will break.Ensure this is documented in release/migration notes and consider whether the API version (
v1.6.3at line 6) should be bumped to signal the breaking change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api-reference/openapiv2/apidocs.swagger.json` around lines 2970 - 2983, The schema change made RuleDefinition.arguments from an object map to an array of NamedArgument is a breaking API change; update the release/migration notes documenting that RuleDefinition.arguments changed shape (map<string, AttributeType> -> array of NamedArgument) and provide migration guidance for clients (how to convert existing maps to the new ordered array), and also bump the public API version currently declared as v1.6.3 to a new version that signals the breaking change (e.g., increment major or next appropriate semver token) in the API metadata and any release tags so consumers are aware of the incompatibility.
🧹 Nitpick comments (2)
pkg/dsl/compiler/compiler.go (1)
614-617: Fragile array-type derivation relies on enum value arithmetic.
attrType + 1assumes the enum pairs are always contiguous (e.g.,STRING=3,STRING_ARRAY=4). If theAttributeTypeenum is ever reordered or extended with gaps, this will silently produce wrong types. This is pre-existing code, but now more prominent with the new argument handling.♻️ Safer alternative using an explicit map
+var arrayTypeMap = map[base.AttributeType]base.AttributeType{ + base.AttributeType_ATTRIBUTE_TYPE_STRING: base.AttributeType_ATTRIBUTE_TYPE_STRING_ARRAY, + base.AttributeType_ATTRIBUTE_TYPE_BOOLEAN: base.AttributeType_ATTRIBUTE_TYPE_BOOLEAN_ARRAY, + base.AttributeType_ATTRIBUTE_TYPE_INTEGER: base.AttributeType_ATTRIBUTE_TYPE_INTEGER_ARRAY, + base.AttributeType_ATTRIBUTE_TYPE_DOUBLE: base.AttributeType_ATTRIBUTE_TYPE_DOUBLE_ARRAY, +} + if tkn.IsArray { - return attrType + 1, nil + arrayType, ok := arrayTypeMap[attrType] + if !ok { + return base.AttributeType_ATTRIBUTE_TYPE_UNSPECIFIED, compileError(tkn.Type.PositionInfo, base.ErrorCode_ERROR_CODE_INVALID_ARGUMENT.String()) + } + return arrayType, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dsl/compiler/compiler.go` around lines 614 - 617, The code in compiler.go uses attrType + 1 to derive the array counterpart when tkn.IsArray is true, which is fragile because it assumes AttributeType enum values are contiguous; replace this arithmetic with an explicit mapping (e.g., a map or switch) from base AttributeType to its array variant inside the function that handles token types (referencing tkn.IsArray and attrType), and return an error if no mapping exists so changes to the AttributeType enum won’t silently produce incorrect types. Ensure the mapping is centralized (or implement a helper like baseToArrayAttributeType(attrType) used where needed) and update callers to handle the possible error return.internal/engines/subject_filter.go (1)
329-379: Consider extracting the duplicated positional argument resolution logic into a shared helper.The argument resolution pattern (building
attrToParam, populating default empty values, querying attributes, and mapping results back viaattrToParam) is nearly identical acrossexpand.go,check.go, andsubject_filter.go. Extracting this into a shared function in theenginespackage would reduce duplication and ensure consistent behavior (including the bounds check) across all three call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engines/subject_filter.go` around lines 329 - 379, There is duplicated positional argument resolution logic across subject_filter.go, expand.go, and check.go (building attrToParam, setting defaults via getEmptyValueForType, querying attributes with engine.dataReader.QueryAttributes and storageContext.NewContextualAttributes(...).QueryAttributes, iterating with database.NewUniqueAttributeIterator, and mapping into the arguments map). Extract this into a single helper in the engines package (e.g., ResolveComputedPositionalArguments) that accepts the context, request, rule (ru), a reference to the arguments map, and any needed readers/contexts, performs the bounds checks and defaults, queries both dataReader and contextual attributes, and fills arguments using attrToParam; replace the duplicated blocks in expand.go, check.go, and subject_filter.go with calls to that helper to ensure consistent 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/engines/check.go`:
- Around line 601-603: The code assumes attrToParam contains next.GetAttribute()
and sets paramName directly, which can yield an empty key if missing; update the
mapping in the function that processes proto attributes (the block using
attrToParam and arguments) to first check the map membership (e.g., lookup with
ok) for next.GetAttribute(), and if the key is absent either skip the attribute
or return a clear error/log (match the behavior used in expand.go) instead of
assigning an empty paramName to arguments; ensure you reference attrToParam,
paramName, and the loop handling next.GetValue() when making the change.
- Around line 557-566: Validate the argument counts before iterating: ensure
len(request.GetArguments()) == len(ru.GetArguments()) and return or handle an
error if they differ, to avoid panics when accessing ru.GetArguments()[i] in the
loop handling request.GetArguments(); add this guard near the start of the
function containing the loop (the block that iterates request.GetArguments()) so
that the switch on arg.Type and use of ru.GetArguments()[i] (used for paramName
and getEmptyValueForType) are only executed when the lengths match.
In `@internal/engines/expand.go`:
- Around line 653-655: The current code maps attributes into the arguments map
using paramName := attrToParam[next.GetAttribute()] which can produce an empty
string for unknown attributes and silently store under arguments[""]; update the
code in the same block (the mapping that uses attrToParam, next.GetAttribute(),
and arguments) to perform a safe lookup (ok := attrToParam[attr]; if !ok then
skip or log/warn and continue) and only assign arguments[paramName] when the
lookup succeeds; ensure you reference attrToParam, next.GetAttribute(), and
arguments in the fix so unknown attributes are not written as an empty-key
entry.
- Around line 588-610: The loop assumes ru.GetArguments()[i] exists for every
request.GetArguments() entry which can panic if request has more args than the
rule; add a bounds check before iterating (or at loop start) to verify
len(request.GetArguments()) <= len(ru.GetArguments()) and, on mismatch, send the
same failure via expandChan (e.g., expandFailResponse with an appropriate
base.ErrorCode) and return; apply the same fix pattern to the analogous loops in
check.go and subject_filter.go to avoid out-of-range panics when accessing
ru.GetArguments()[i], and ensure you use the same error handling path used
elsewhere (expandFailResponse/expandChan) to keep behavior consistent.
In `@internal/engines/subject_filter.go`:
- Around line 377-379: The code assumes attrToParam[next.GetAttribute()] always
exists; update the block that maps attributes to parameters (using attrToParam,
next.GetAttribute(), and arguments) to first check for the key presence (e.g.,
val, ok := attrToParam[next.GetAttribute()]) and only set arguments[paramName] =
utils.ConvertProtoAnyToInterface(next.GetValue()) when ok is true; if the key is
missing, skip or handle it the same way as expand.go/check.go (e.g., ignore the
attribute or surface an error) to avoid panics from missing map keys.
- Around line 333-342: The loop over request.GetArguments() uses
ru.GetArguments()[i] without bounds checking and can panic; before using
ru.GetArguments()[i] (or at start of the loop), validate that i <
len(ru.GetArguments()) and handle the mismatch (return an error or skip) so
accessing ru.GetArguments()[i] is safe; update the block in subject_filter.go
where request.GetArguments() is iterated (the switch on arg.Type and uses of
attrName, paramName, getEmptyValueForType, and attrToParam) to perform this
bounds check and fail-fast or continue appropriately.
---
Outside diff comments:
In `@docs/api-reference/openapiv2/apidocs.swagger.json`:
- Around line 2970-2983: The schema change made RuleDefinition.arguments from an
object map to an array of NamedArgument is a breaking API change; update the
release/migration notes documenting that RuleDefinition.arguments changed shape
(map<string, AttributeType> -> array of NamedArgument) and provide migration
guidance for clients (how to convert existing maps to the new ordered array),
and also bump the public API version currently declared as v1.6.3 to a new
version that signals the breaking change (e.g., increment major or next
appropriate semver token) in the API metadata and any release tags so consumers
are aware of the incompatibility.
---
Nitpick comments:
In `@internal/engines/subject_filter.go`:
- Around line 329-379: There is duplicated positional argument resolution logic
across subject_filter.go, expand.go, and check.go (building attrToParam, setting
defaults via getEmptyValueForType, querying attributes with
engine.dataReader.QueryAttributes and
storageContext.NewContextualAttributes(...).QueryAttributes, iterating with
database.NewUniqueAttributeIterator, and mapping into the arguments map).
Extract this into a single helper in the engines package (e.g.,
ResolveComputedPositionalArguments) that accepts the context, request, rule
(ru), a reference to the arguments map, and any needed readers/contexts,
performs the bounds checks and defaults, queries both dataReader and contextual
attributes, and fills arguments using attrToParam; replace the duplicated blocks
in expand.go, check.go, and subject_filter.go with calls to that helper to
ensure consistent behavior.
In `@pkg/dsl/compiler/compiler.go`:
- Around line 614-617: The code in compiler.go uses attrType + 1 to derive the
array counterpart when tkn.IsArray is true, which is fragile because it assumes
AttributeType enum values are contiguous; replace this arithmetic with an
explicit mapping (e.g., a map or switch) from base AttributeType to its array
variant inside the function that handles token types (referencing tkn.IsArray
and attrType), and return an error if no mapping exists so changes to the
AttributeType enum won’t silently produce incorrect types. Ensure the mapping is
centralized (or implement a helper like baseToArrayAttributeType(attrType) used
where needed) and update callers to handle the possible error return.
Comment on lines
+557
to
+566
| for i, arg := range request.GetArguments() { | ||
| switch actualArg := arg.Type.(type) { | ||
| case *base.Argument_ComputedAttribute: | ||
| // Handle computed attributes: Set them to a default empty value. | ||
| attrName := actualArg.ComputedAttribute.GetName() | ||
| emptyValue := getEmptyValueForType(ru.GetArguments()[attrName]) | ||
| arguments[attrName] = emptyValue | ||
| // Get the parameter name from the rule definition at position i | ||
| paramName := ru.GetArguments()[i].GetName() | ||
| emptyValue := getEmptyValueForType(ru.GetArguments()[i].GetType()) | ||
| arguments[paramName] = emptyValue | ||
| attrToParam[attrName] = paramName |
There was a problem hiding this comment.
Same missing bounds check as in expand.go: ru.GetArguments()[i] can panic.
Add a guard before the loop to validate len(request.GetArguments()) == len(ru.GetArguments()), consistent with the fix recommended in expand.go.
🛡️ Proposed fix
+ if len(request.GetArguments()) != len(ru.GetArguments()) {
+ return denied(emptyResponseMetadata()), errors.New(base.ErrorCode_ERROR_CODE_MISSING_ARGUMENT.String())
+ }
+
// Iterate over request arguments to classify and process them.
for i, arg := range request.GetArguments() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/engines/check.go` around lines 557 - 566, Validate the argument
counts before iterating: ensure len(request.GetArguments()) ==
len(ru.GetArguments()) and return or handle an error if they differ, to avoid
panics when accessing ru.GetArguments()[i] in the loop handling
request.GetArguments(); add this guard near the start of the function containing
the loop (the block that iterates request.GetArguments()) so that the switch on
arg.Type and use of ru.GetArguments()[i] (used for paramName and
getEmptyValueForType) are only executed when the lengths match.
Comment on lines
+601
to
+603
| // Map the attribute value to the parameter name | ||
| paramName := attrToParam[next.GetAttribute()] | ||
| arguments[paramName] = utils.ConvertProtoAnyToInterface(next.GetValue()) |
There was a problem hiding this comment.
Guard attrToParam lookup against missing keys.
Same issue as in expand.go — if the attribute name isn't in the map, paramName will be empty.
🛡️ Proposed fix
- paramName := attrToParam[next.GetAttribute()]
- arguments[paramName] = utils.ConvertProtoAnyToInterface(next.GetValue())
+ paramName, ok := attrToParam[next.GetAttribute()]
+ if !ok {
+ continue
+ }
+ arguments[paramName] = utils.ConvertProtoAnyToInterface(next.GetValue())📝 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
| // Map the attribute value to the parameter name | |
| paramName := attrToParam[next.GetAttribute()] | |
| arguments[paramName] = utils.ConvertProtoAnyToInterface(next.GetValue()) | |
| // Map the attribute value to the parameter name | |
| paramName, ok := attrToParam[next.GetAttribute()] | |
| if !ok { | |
| continue | |
| } | |
| arguments[paramName] = utils.ConvertProtoAnyToInterface(next.GetValue()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/engines/check.go` around lines 601 - 603, The code assumes
attrToParam contains next.GetAttribute() and sets paramName directly, which can
yield an empty key if missing; update the mapping in the function that processes
proto attributes (the block using attrToParam and arguments) to first check the
map membership (e.g., lookup with ok) for next.GetAttribute(), and if the key is
absent either skip the attribute or return a clear error/log (match the behavior
used in expand.go) instead of assigning an empty paramName to arguments; ensure
you reference attrToParam, paramName, and the loop handling next.GetValue() when
making the change.
Comment on lines
+588
to
610
| for i, arg := range request.GetArguments() { | ||
| switch actualArg := arg.Type.(type) { // Switch on the type of the argument. | ||
| case *base.Argument_ComputedAttribute: // If the argument is a ComputedAttribute... | ||
| attrName := actualArg.ComputedAttribute.GetName() // get the name of the attribute. | ||
|
|
||
| // Get the parameter name from the rule definition at position i | ||
| paramName := ru.GetArguments()[i].GetName() | ||
|
|
||
| // Get the empty value for the attribute type. | ||
| emptyValue, err := getEmptyProtoValueForType(ru.GetArguments()[attrName]) | ||
| emptyValue, err := getEmptyProtoValueForType(ru.GetArguments()[i].GetType()) | ||
| if err != nil { | ||
| expandChan <- expandFailResponse(errors.New(base.ErrorCode_ERROR_CODE_TYPE_CONVERSATION.String())) | ||
| return | ||
| } | ||
|
|
||
| // Set the empty value in the arguments map. | ||
| arguments[attrName] = emptyValue | ||
| // Set the empty value in the arguments map using the parameter name. | ||
| arguments[paramName] = emptyValue | ||
|
|
||
| // Store the mapping from attribute name to parameter name. | ||
| attrToParam[attrName] = paramName | ||
|
|
||
| // Append the attribute name to the attributes slice. | ||
| attributes = append(attributes, attrName) |
There was a problem hiding this comment.
Missing bounds check: ru.GetArguments()[i] will panic if argument counts mismatch.
If len(request.GetArguments()) > len(ru.GetArguments()), the index i at lines 594 and 597 will be out of range, causing a runtime panic. The compiler validates argument count at schema compile time, but runtime requests are not validated here. The same issue exists in check.go and subject_filter.go.
🛡️ Proposed fix: add a bounds check before the loop
+ // Validate argument count matches the rule definition
+ if len(request.GetArguments()) != len(ru.GetArguments()) {
+ expandChan <- expandFailResponse(errors.New(base.ErrorCode_ERROR_CODE_MISSING_ARGUMENT.String()))
+ return
+ }
+
// For each argument in the call...
for i, arg := range request.GetArguments() {📝 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
| for i, arg := range request.GetArguments() { | |
| switch actualArg := arg.Type.(type) { // Switch on the type of the argument. | |
| case *base.Argument_ComputedAttribute: // If the argument is a ComputedAttribute... | |
| attrName := actualArg.ComputedAttribute.GetName() // get the name of the attribute. | |
| // Get the parameter name from the rule definition at position i | |
| paramName := ru.GetArguments()[i].GetName() | |
| // Get the empty value for the attribute type. | |
| emptyValue, err := getEmptyProtoValueForType(ru.GetArguments()[attrName]) | |
| emptyValue, err := getEmptyProtoValueForType(ru.GetArguments()[i].GetType()) | |
| if err != nil { | |
| expandChan <- expandFailResponse(errors.New(base.ErrorCode_ERROR_CODE_TYPE_CONVERSATION.String())) | |
| return | |
| } | |
| // Set the empty value in the arguments map. | |
| arguments[attrName] = emptyValue | |
| // Set the empty value in the arguments map using the parameter name. | |
| arguments[paramName] = emptyValue | |
| // Store the mapping from attribute name to parameter name. | |
| attrToParam[attrName] = paramName | |
| // Append the attribute name to the attributes slice. | |
| attributes = append(attributes, attrName) | |
| // Validate argument count matches the rule definition | |
| if len(request.GetArguments()) != len(ru.GetArguments()) { | |
| expandChan <- expandFailResponse(errors.New(base.ErrorCode_ERROR_CODE_MISSING_ARGUMENT.String())) | |
| return | |
| } | |
| for i, arg := range request.GetArguments() { | |
| switch actualArg := arg.Type.(type) { // Switch on the type of the argument. | |
| case *base.Argument_ComputedAttribute: // If the argument is a ComputedAttribute... | |
| attrName := actualArg.ComputedAttribute.GetName() // get the name of the attribute. | |
| // Get the parameter name from the rule definition at position i | |
| paramName := ru.GetArguments()[i].GetName() | |
| // Get the empty value for the attribute type. | |
| emptyValue, err := getEmptyProtoValueForType(ru.GetArguments()[i].GetType()) | |
| if err != nil { | |
| expandChan <- expandFailResponse(errors.New(base.ErrorCode_ERROR_CODE_TYPE_CONVERSATION.String())) | |
| return | |
| } | |
| // Set the empty value in the arguments map using the parameter name. | |
| arguments[paramName] = emptyValue | |
| // Store the mapping from attribute name to parameter name. | |
| attrToParam[attrName] = paramName | |
| // Append the attribute name to the attributes slice. | |
| attributes = append(attributes, attrName) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/engines/expand.go` around lines 588 - 610, The loop assumes
ru.GetArguments()[i] exists for every request.GetArguments() entry which can
panic if request has more args than the rule; add a bounds check before
iterating (or at loop start) to verify len(request.GetArguments()) <=
len(ru.GetArguments()) and, on mismatch, send the same failure via expandChan
(e.g., expandFailResponse with an appropriate base.ErrorCode) and return; apply
the same fix pattern to the analogous loops in check.go and subject_filter.go to
avoid out-of-range panics when accessing ru.GetArguments()[i], and ensure you
use the same error handling path used elsewhere (expandFailResponse/expandChan)
to keep behavior consistent.
Comment on lines
+653
to
+655
| // Map the attribute value to the parameter name | ||
| paramName := attrToParam[next.GetAttribute()] | ||
| arguments[paramName] = next.GetValue() |
There was a problem hiding this comment.
attrToParam lookup may yield an empty string for unknown attribute names.
If next.GetAttribute() returns an attribute name not present in the attrToParam map (e.g., from an unexpected contextual attribute), paramName will be an empty string, and the value will be silently stored under arguments[""]. Consider guarding with a map lookup check.
🛡️ Proposed fix
- paramName := attrToParam[next.GetAttribute()]
- arguments[paramName] = next.GetValue()
+ paramName, ok := attrToParam[next.GetAttribute()]
+ if !ok {
+ continue
+ }
+ arguments[paramName] = next.GetValue()📝 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
| // Map the attribute value to the parameter name | |
| paramName := attrToParam[next.GetAttribute()] | |
| arguments[paramName] = next.GetValue() | |
| // Map the attribute value to the parameter name | |
| paramName, ok := attrToParam[next.GetAttribute()] | |
| if !ok { | |
| continue | |
| } | |
| arguments[paramName] = next.GetValue() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/engines/expand.go` around lines 653 - 655, The current code maps
attributes into the arguments map using paramName :=
attrToParam[next.GetAttribute()] which can produce an empty string for unknown
attributes and silently store under arguments[""]; update the code in the same
block (the mapping that uses attrToParam, next.GetAttribute(), and arguments) to
perform a safe lookup (ok := attrToParam[attr]; if !ok then skip or log/warn and
continue) and only assign arguments[paramName] when the lookup succeeds; ensure
you reference attrToParam, next.GetAttribute(), and arguments in the fix so
unknown attributes are not written as an empty-key entry.
Comment on lines
+333
to
+342
| for i, arg := range request.GetArguments() { | ||
| switch actualArg := arg.Type.(type) { | ||
| case *base.Argument_ComputedAttribute: | ||
| // Handle computed attributes: Set them to a default empty value. | ||
| attrName := actualArg.ComputedAttribute.GetName() | ||
| emptyValue := getEmptyValueForType(ru.GetArguments()[attrName]) | ||
| arguments[attrName] = emptyValue | ||
| // Get the parameter name from the rule definition at position i | ||
| paramName := ru.GetArguments()[i].GetName() | ||
| emptyValue := getEmptyValueForType(ru.GetArguments()[i].GetType()) | ||
| arguments[paramName] = emptyValue | ||
| attrToParam[attrName] = paramName |
There was a problem hiding this comment.
Same missing bounds check: ru.GetArguments()[i] can panic — third occurrence.
Identical to the issue flagged in expand.go and check.go. Add a length validation before the loop.
🛡️ Proposed fix
+ if len(request.GetArguments()) != len(ru.GetArguments()) {
+ return subjectFilterEmpty(), errors.New(base.ErrorCode_ERROR_CODE_MISSING_ARGUMENT.String())
+ }
+
// Iterate over request arguments to classify and process them.
for i, arg := range request.GetArguments() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/engines/subject_filter.go` around lines 333 - 342, The loop over
request.GetArguments() uses ru.GetArguments()[i] without bounds checking and can
panic; before using ru.GetArguments()[i] (or at start of the loop), validate
that i < len(ru.GetArguments()) and handle the mismatch (return an error or
skip) so accessing ru.GetArguments()[i] is safe; update the block in
subject_filter.go where request.GetArguments() is iterated (the switch on
arg.Type and uses of attrName, paramName, getEmptyValueForType, and attrToParam)
to perform this bounds check and fail-fast or continue appropriately.
Comment on lines
+377
to
+379
| // Map the attribute value to the parameter name | ||
| paramName := attrToParam[next.GetAttribute()] | ||
| arguments[paramName] = utils.ConvertProtoAnyToInterface(next.GetValue()) |
There was a problem hiding this comment.
Guard attrToParam lookup against missing keys — same as expand.go and check.go.
🛡️ Proposed fix
- paramName := attrToParam[next.GetAttribute()]
- arguments[paramName] = utils.ConvertProtoAnyToInterface(next.GetValue())
+ paramName, ok := attrToParam[next.GetAttribute()]
+ if !ok {
+ continue
+ }
+ arguments[paramName] = utils.ConvertProtoAnyToInterface(next.GetValue())📝 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
| // Map the attribute value to the parameter name | |
| paramName := attrToParam[next.GetAttribute()] | |
| arguments[paramName] = utils.ConvertProtoAnyToInterface(next.GetValue()) | |
| // Map the attribute value to the parameter name | |
| paramName, ok := attrToParam[next.GetAttribute()] | |
| if !ok { | |
| continue | |
| } | |
| arguments[paramName] = utils.ConvertProtoAnyToInterface(next.GetValue()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/engines/subject_filter.go` around lines 377 - 379, The code assumes
attrToParam[next.GetAttribute()] always exists; update the block that maps
attributes to parameters (using attrToParam, next.GetAttribute(), and arguments)
to first check for the key presence (e.g., val, ok :=
attrToParam[next.GetAttribute()]) and only set arguments[paramName] =
utils.ConvertProtoAnyToInterface(next.GetValue()) when ok is true; if the key is
missing, skip or handle it the same way as expand.go/check.go (e.g., ignore the
attribute or surface an error) to avoid panics from missing map keys.
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 by CodeRabbit
Release Notes