Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces property-based matching for graph edge endpoints. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 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)
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 |
zinic
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: 5
🧹 Nitpick comments (1)
cmd/api/src/services/graphify/ingestrelationships_integration_test.go (1)
380-380: Prefer integration assertions throughIngestRelationshipsinstead of the internal builder.These call-site swaps now exercise
buildIngestionUpdateBatchdirectly and bypass submission/changelog behavior inIngestRelationships. Consider keeping this helper in unit tests and usingIngestRelationshipsfor integration coverage.Also applies to: 420-420, 450-450, 480-480, 510-510, 540-540, 570-570, 610-610, 649-649, 687-687, 725-725, 755-755, 785-785, 852-852
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/services/graphify/ingestrelationships_integration_test.go` at line 380, Replace direct uses of the internal builder buildIngestionUpdateBatch in the integration test with the public IngestRelationships entry point so the test exercises submission/changelog behavior; specifically, where you currently call buildIngestionUpdateBatch(ingestContext, []ein.IngestibleRelationship{ingestibleRel}, graph.EmptyKind) (and the other similar call sites), invoke IngestRelationships with the same ingestContext and payload (wrapping ingestibleRel as needed) and assert against the results and side-effects produced by IngestRelationships rather than the builder, keeping buildIngestionUpdateBatch for unit tests only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/services/graphify/convertors.go`:
- Around line 68-77: The conversion currently unconditionally calls
strings.ToUpper on entity.Start.Value and entity.End.Value when building
ein.IngestibleEndpoint, which mutates values used with match_by: property;
change the logic in convertors.go so Value is only uppercased when the
endpoint's MatchBy is not the property-match strategy: inspect
entity.Start.MatchBy / entity.End.MatchBy (or the result of
ein.IngestMatchStrategy(...)) and if it represents the property match leave
Value unchanged, otherwise use strings.ToUpper; update both constructions of
ein.IngestibleEndpoint (for Start and End) to apply this conditional
normalization.
In `@cmd/api/src/services/graphify/ingestrelationships.go`:
- Around line 194-233: buildIngestionUpdateBatch currently can panic when
rel.RelProps is nil and emits updates with empty/invalid identity properties;
before mutating rel.RelProps or building updates, validate each rel: if
rel.RelProps == nil initialize it (or add an error to errs via
errorlist.Builder) and set LastSeen, and verify rel.Source.IdentityProperty(),
rel.Source.Value, rel.Target.IdentityProperty(), and rel.Target.Value are
non-empty; if any identity property/value is empty add a descriptive error to
errs and skip producing an update for that rel (or return an error), ensuring
you use the existing errs builder so errs.Build() returns collected errors;
update code paths around rel.RelProps, rel.Source, rel.Target and the
construction of graph.RelationshipUpdate (functions/types:
buildIngestionUpdateBatch, IngestContext, MergeNodeKinds, graph.PrepareNode,
graph.PrepareRelationship) to perform these checks and filtering before
appending to updates.
In `@packages/go/chow/ingestvalidator/jsonschema/edge.json`:
- Around line 9-18: Add a JSON Schema conditional so that when the "match_by"
field equals "property" the "property" field becomes required: update the
"match_by"/"property" block in edge.json to include an "if": {"properties":
{"match_by": {"const": "property"}}}, "then": {"required": ["property"]} (and
any corresponding "else" behavior if needed); apply the same conditional guard
to the second occurrence of the same pattern (lines 33-42 equivalent) so both
schemas enforce that "property" must be present when match_by is "property".
- Around line 20-21: The JSON schema for endpoint "value" currently permits
non-string types which our ingest models (EdgeEndpoint.Value and
IngestibleEndpoint.Value) can't deserialize; update the schema entries that
define the endpoint "value" (where "type" is currently
["number","string","boolean","array"]) to restrict the type to "string" only so
validation aligns with the Go models, or alternately update EdgeEndpoint.Value
and IngestibleEndpoint.Value in the Go code to accept a typed representation if
you intend to support non-string values end-to-end; ensure the schema change is
applied to all occurrences corresponding to the endpoint value definitions
referenced by EdgeEndpoint.Value and IngestibleEndpoint.Value.
In `@packages/go/ein/models.go`:
- Around line 56-67: The IdentityProperty method on IngestibleEndpoint currently
returns an empty string when s.MatchBy is unset; change its default branch to
return the ID property instead of "" so zero-value endpoints use the default ID
behavior. In other words, in the IdentityProperty() function (handling MatchBy,
MatchByID, MatchByName, MatchByGenericProperty), ensure the fallback/else case
returns common.ObjectID.String() (same as MatchByID) rather than an empty string
so identity keys are never emitted empty.
---
Nitpick comments:
In `@cmd/api/src/services/graphify/ingestrelationships_integration_test.go`:
- Line 380: Replace direct uses of the internal builder
buildIngestionUpdateBatch in the integration test with the public
IngestRelationships entry point so the test exercises submission/changelog
behavior; specifically, where you currently call
buildIngestionUpdateBatch(ingestContext,
[]ein.IngestibleRelationship{ingestibleRel}, graph.EmptyKind) (and the other
similar call sites), invoke IngestRelationships with the same ingestContext and
payload (wrapping ingestibleRel as needed) and assert against the results and
side-effects produced by IngestRelationships rather than the builder, keeping
buildIngestionUpdateBatch for unit tests only.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/api/src/services/graphify/convertors.gocmd/api/src/services/graphify/ingestrelationships.gocmd/api/src/services/graphify/ingestrelationships_integration_test.gopackages/go/chow/ingestvalidator/jsonschema/edge.jsonpackages/go/ein/incoming_models.gopackages/go/ein/models.go
Comment on lines
+68
to
+77
| Property: entity.Start.Property, | ||
| Value: strings.ToUpper(entity.Start.Value), | ||
| MatchBy: ein.IngestMatchStrategy(entity.Start.MatchBy), | ||
| Kind: graph.StringKind(entity.Start.Kind), | ||
| }, | ||
| ein.IngestibleEndpoint{ | ||
| Value: strings.ToUpper(entity.End.Value), | ||
| MatchBy: ein.IngestMatchStrategy(entity.End.MatchBy), | ||
| Kind: graph.StringKind(entity.End.Kind), | ||
| Property: entity.End.Property, | ||
| Value: strings.ToUpper(entity.End.Value), | ||
| MatchBy: ein.IngestMatchStrategy(entity.End.MatchBy), | ||
| Kind: graph.StringKind(entity.End.Kind), |
Contributor
There was a problem hiding this comment.
Do not uppercase values for match_by: property.
On Line 69 and Line 75, value normalization is unconditional. That mutates property-match values and can cause false misses for case-sensitive properties.
💡 Proposed fix
func ConvertGenericEdge(entity ein.GenericEdge, converted *ConvertedData) error {
+ startMatchBy := ein.IngestMatchStrategy(entity.Start.MatchBy)
+ startValue := entity.Start.Value
+ if startMatchBy != ein.MatchByGenericProperty {
+ startValue = strings.ToUpper(startValue)
+ }
+
+ endMatchBy := ein.IngestMatchStrategy(entity.End.MatchBy)
+ endValue := entity.End.Value
+ if endMatchBy != ein.MatchByGenericProperty {
+ endValue = strings.ToUpper(endValue)
+ }
+
ingestibleRel := ein.NewIngestibleRelationship(
ein.IngestibleEndpoint{
Property: entity.Start.Property,
- Value: strings.ToUpper(entity.Start.Value),
- MatchBy: ein.IngestMatchStrategy(entity.Start.MatchBy),
+ Value: startValue,
+ MatchBy: startMatchBy,
Kind: graph.StringKind(entity.Start.Kind),
},
ein.IngestibleEndpoint{
Property: entity.End.Property,
- Value: strings.ToUpper(entity.End.Value),
- MatchBy: ein.IngestMatchStrategy(entity.End.MatchBy),
+ Value: endValue,
+ MatchBy: endMatchBy,
Kind: graph.StringKind(entity.End.Kind),
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/api/src/services/graphify/convertors.go` around lines 68 - 77, The
conversion currently unconditionally calls strings.ToUpper on entity.Start.Value
and entity.End.Value when building ein.IngestibleEndpoint, which mutates values
used with match_by: property; change the logic in convertors.go so Value is only
uppercased when the endpoint's MatchBy is not the property-match strategy:
inspect entity.Start.MatchBy / entity.End.MatchBy (or the result of
ein.IngestMatchStrategy(...)) and if it represents the property match leave
Value unchanged, otherwise use strings.ToUpper; update both constructions of
ein.IngestibleEndpoint (for Start and End) to apply this conditional
normalization.
Comment on lines
+194
to
233
| func buildIngestionUpdateBatch(batch *IngestContext, rels []ein.IngestibleRelationship, sourceKind graph.Kind) ([]graph.RelationshipUpdate, error) { | ||
| var ( | ||
| filters = make([]graph.Criteria, 0, len(seen)) | ||
| buildFilter = func(key endpointKey) graph.Criteria { | ||
| var criteria []graph.Criteria | ||
|
|
||
| criteria = append(criteria, query.Equals(query.NodeProperty(common.Name.String()), key.Name)) | ||
| if key.Kind != "" { | ||
| criteria = append(criteria, query.Kind(query.Node(), graph.StringKind(key.Kind))) | ||
| } | ||
| return query.And(criteria...) | ||
| } | ||
| updates []graph.RelationshipUpdate | ||
| errs = errorlist.NewBuilder() | ||
| ) | ||
|
|
||
| // aggregate all Name:Kind pairs in 1 DAWGs query for 1 round trip | ||
| for key := range seen { | ||
| filters = append(filters, buildFilter(key)) | ||
| } | ||
|
|
||
| var ( | ||
| resolved = map[endpointKey]string{} | ||
| ambiguous = map[endpointKey]bool{} | ||
| ) | ||
|
|
||
| if err := batch.Nodes().Filter(query.Or(filters...)).Fetch( | ||
| func(cursor graph.Cursor[*graph.Node]) error { | ||
|
|
||
| for node := range cursor.Chan() { | ||
| nameVal, _ := node.Properties.Get(common.Name.String()).String() | ||
| objectID, err := node.Properties.Get(string(common.ObjectID)).String() | ||
| if err != nil || objectID == "" { | ||
| slog.Warn("Matched node missing objectid", | ||
| slog.String("name", nameVal), | ||
| slog.Any("kinds", node.Kinds)) | ||
| continue | ||
| } | ||
|
|
||
| // edge case: resolve an empty key to match endpoints that provide no Kind filter | ||
| node.Kinds = append(node.Kinds, graph.EmptyKind) | ||
|
|
||
| // resolve all names found to objectids, | ||
| // record ambiguous matches (when more than one match is found, we cannot disambiguate the requested node and must skip the update) | ||
| for _, kind := range node.Kinds { | ||
| key := endpointKey{Name: strings.ToUpper(nameVal), Kind: kind.String()} | ||
| if existingID, exists := resolved[key]; exists && existingID != objectID { | ||
| ambiguous[key] = true | ||
| } else { | ||
| resolved[key] = objectID | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| }, | ||
| ); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // remove ambiguous matches | ||
| for key := range ambiguous { | ||
| delete(resolved, key) | ||
| } | ||
|
|
||
| return resolved, nil | ||
| } | ||
| for _, rel := range rels { | ||
| rel.RelProps[common.LastSeen.String()] = batch.IngestTime | ||
|
|
||
| // resolveRelationships transforms a list of ingestible relationships into a | ||
| // slice of graph.RelationshipUpdate objects, suitable for ingestion into the | ||
| // graph database. | ||
| // | ||
| // The function resolves all source and target endpoints to their corresponding | ||
| // object IDs if MatchByName is set on an endpoint. Relationships with unresolved | ||
| // or ambiguous endpoints are skipped and logged with a warning. | ||
| // | ||
| // The identityKind parameter determines the identity kind used for both start | ||
| // and end nodes if provided. eg. ad.Base and az.Base are used for *hound collections, and generic ingest has no base kind. | ||
| // | ||
| // Each resolved relationship is stamped with the current UTC timestamp as the "last seen" property. | ||
| // | ||
| // Returns a slice of valid relationship updates or an error if resolution fails. | ||
| func resolveRelationships(batch *IngestContext, rels []ein.IngestibleRelationship, sourceKind graph.Kind) ([]graph.RelationshipUpdate, error) { | ||
| if cache, err := resolveAllEndpointsByName(batch.Batch, rels); err != nil { | ||
| return nil, err | ||
| } else { | ||
| var ( | ||
| updates []graph.RelationshipUpdate | ||
| errs = errorlist.NewBuilder() | ||
| startIdentityProperty = rel.Source.IdentityProperty() | ||
| startKinds = MergeNodeKinds(sourceKind, rel.Source.Kind) | ||
| startProperties = graph.AsProperties(map[string]any{ | ||
| startIdentityProperty: rel.Source.Value, | ||
| common.LastSeen.String(): batch.IngestTime, | ||
| }) | ||
|
|
||
| endIdentityProperty = rel.Target.IdentityProperty() | ||
| endKinds = MergeNodeKinds(sourceKind, rel.Target.Kind) | ||
| endProperties = graph.AsProperties(map[string]any{ | ||
| endIdentityProperty: rel.Target.Value, | ||
| common.LastSeen.String(): batch.IngestTime, | ||
| }) | ||
| ) | ||
|
|
||
| for _, rel := range rels { | ||
| srcID, srcOK := resolveEndpointID(rel.Source, cache) | ||
| targetID, targetOK := resolveEndpointID(rel.Target, cache) | ||
|
|
||
| if !srcOK || !targetOK { | ||
| slog.Warn("Skipping unresolved relationship", | ||
| slog.String("source", rel.Source.Value), | ||
| slog.String("target", rel.Target.Value), | ||
| slog.Bool("resolved_source", srcOK), | ||
| slog.Bool("resolved_target", targetOK), | ||
| slog.String("type", rel.RelType.String())) | ||
| errs.Add( | ||
| IngestUserDataError{ | ||
| Msg: fmt.Sprintf("skipping invalid relationship. unable to resolve endpoints. source: %s, target: %s", rel.Source.Value, rel.Target.Value), | ||
| }, | ||
| ) | ||
| continue | ||
| } | ||
|
|
||
| rel.RelProps[common.LastSeen.String()] = batch.IngestTime | ||
|
|
||
| startKinds := MergeNodeKinds(sourceKind, rel.Source.Kind) | ||
| endKinds := MergeNodeKinds(sourceKind, rel.Target.Kind) | ||
|
|
||
| update := graph.RelationshipUpdate{ | ||
| Start: graph.PrepareNode(graph.AsProperties(graph.PropertyMap{ | ||
| common.ObjectID: srcID, | ||
| common.LastSeen: batch.IngestTime, | ||
| }), startKinds...), | ||
| StartIdentityProperties: []string{common.ObjectID.String()}, | ||
| StartIdentityKind: sourceKind, | ||
| End: graph.PrepareNode(graph.AsProperties(graph.PropertyMap{ | ||
| common.ObjectID: targetID, | ||
| common.LastSeen: batch.IngestTime, | ||
| }), endKinds...), | ||
| EndIdentityKind: sourceKind, | ||
| EndIdentityProperties: []string{common.ObjectID.String()}, | ||
| Relationship: graph.PrepareRelationship(graph.AsProperties(rel.RelProps), rel.RelType), | ||
| } | ||
|
|
||
| updates = append(updates, update) | ||
| update := graph.RelationshipUpdate{ | ||
| Start: graph.PrepareNode(startProperties, startKinds...), | ||
| StartIdentityKind: sourceKind, | ||
| StartIdentityProperties: []string{startIdentityProperty}, | ||
| End: graph.PrepareNode(endProperties, endKinds...), | ||
| EndIdentityKind: sourceKind, | ||
| EndIdentityProperties: []string{endIdentityProperty}, | ||
| Relationship: graph.PrepareRelationship(graph.AsProperties(rel.RelProps), rel.RelType), | ||
| } | ||
|
|
||
| return updates, errs.Build() | ||
| } | ||
| } | ||
|
|
||
| func resolveEndpointID(endpoint ein.IngestibleEndpoint, cache map[endpointKey]string) (string, bool) { | ||
| if endpoint.MatchBy == ein.MatchByName { | ||
| key := endpointKey{ | ||
| Name: strings.ToUpper(endpoint.Value), | ||
| Kind: "", | ||
| } | ||
| if endpoint.Kind != nil { | ||
| key.Kind = endpoint.Kind.String() | ||
| } | ||
| id, ok := cache[key] | ||
| return id, ok | ||
| updates = append(updates, update) | ||
| } | ||
|
|
||
| // Fallback to raw value if matching by ID | ||
| return endpoint.Value, endpoint.Value != "" | ||
| return updates, errs.Build() | ||
| } |
Contributor
There was a problem hiding this comment.
buildIngestionUpdateBatch never rejects invalid endpoints and can panic on nil RelProps.
Line 197 initializes an error builder but no errors are added, and Line 201 can panic if rel.RelProps is nil. Also, empty identity properties/values are not filtered, so invalid updates are still emitted.
💡 Proposed fix
func buildIngestionUpdateBatch(batch *IngestContext, rels []ein.IngestibleRelationship, sourceKind graph.Kind) ([]graph.RelationshipUpdate, error) {
var (
updates []graph.RelationshipUpdate
errs = errorlist.NewBuilder()
)
for _, rel := range rels {
+ if rel.RelProps == nil {
+ rel.RelProps = make(map[string]any)
+ }
+
+ startIdentityProperty := rel.Source.IdentityProperty()
+ endIdentityProperty := rel.Target.IdentityProperty()
+ if rel.Source.Value == "" || rel.Target.Value == "" || startIdentityProperty == "" || endIdentityProperty == "" {
+ errs.Add(fmt.Errorf("skipping invalid relationship: invalid endpoint identity"))
+ continue
+ }
+
rel.RelProps[common.LastSeen.String()] = batch.IngestTime
var (
- startIdentityProperty = rel.Source.IdentityProperty()
startKinds = MergeNodeKinds(sourceKind, rel.Source.Kind)
startProperties = graph.AsProperties(map[string]any{
startIdentityProperty: rel.Source.Value,
common.LastSeen.String(): batch.IngestTime,
})
- endIdentityProperty = rel.Target.IdentityProperty()
endKinds = MergeNodeKinds(sourceKind, rel.Target.Kind)
endProperties = graph.AsProperties(map[string]any{
endIdentityProperty: rel.Target.Value,
common.LastSeen.String(): batch.IngestTime,
})
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/api/src/services/graphify/ingestrelationships.go` around lines 194 - 233,
buildIngestionUpdateBatch currently can panic when rel.RelProps is nil and emits
updates with empty/invalid identity properties; before mutating rel.RelProps or
building updates, validate each rel: if rel.RelProps == nil initialize it (or
add an error to errs via errorlist.Builder) and set LastSeen, and verify
rel.Source.IdentityProperty(), rel.Source.Value, rel.Target.IdentityProperty(),
and rel.Target.Value are non-empty; if any identity property/value is empty add
a descriptive error to errs and skip producing an update for that rel (or return
an error), ensuring you use the existing errs builder so errs.Build() returns
collected errors; update code paths around rel.RelProps, rel.Source, rel.Target
and the construction of graph.RelationshipUpdate (functions/types:
buildIngestionUpdateBatch, IngestContext, MergeNodeKinds, graph.PrepareNode,
graph.PrepareRelationship) to perform these checks and filtering before
appending to updates.
Comment on lines
9
to
+18
| "match_by": { | ||
| "type": "string", | ||
| "enum": ["id", "name"], | ||
| "enum": ["id", "name", "property"], | ||
| "default": "id", | ||
| "description": "Whether to match the start node by its unique object ID or by its name property." | ||
| "description": "Matching method to use. Addressing a node by its 'objectid' or 'name' property which are indexed. All other lookups may use the 'property' match_by option and specify a property name to target." | ||
| }, | ||
| "value": { | ||
| "property": { | ||
| "type": "string", | ||
| "description": "Optional. The property name to match against when using the 'property' match_by method." | ||
| }, |
Contributor
There was a problem hiding this comment.
Require property when match_by is "property".
Right now property is optional for the property strategy. That permits invalid references (no identity property key) to pass schema validation.
💡 Proposed schema guard
"start": {
"type": "object",
"properties": {
@@
},
+ "allOf": [
+ {
+ "if": {
+ "properties": { "match_by": { "const": "property" } },
+ "required": ["match_by"]
+ },
+ "then": { "required": ["property", "value"] }
+ }
+ ],
"required": ["value"]
},
@@
"end": {
"type": "object",
"properties": {
@@
},
+ "allOf": [
+ {
+ "if": {
+ "properties": { "match_by": { "const": "property" } },
+ "required": ["match_by"]
+ },
+ "then": { "required": ["property", "value"] }
+ }
+ ],
"required": ["value"]
},Also applies to: 33-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/go/chow/ingestvalidator/jsonschema/edge.json` around lines 9 - 18,
Add a JSON Schema conditional so that when the "match_by" field equals
"property" the "property" field becomes required: update the
"match_by"/"property" block in edge.json to include an "if": {"properties":
{"match_by": {"const": "property"}}}, "then": {"required": ["property"]} (and
any corresponding "else" behavior if needed); apply the same conditional guard
to the second occurrence of the same pattern (lines 33-42 equivalent) so both
schemas enforce that "property" must be present when match_by is "property".
Comment on lines
+20
to
21
| "type": ["number", "string", "boolean", "array"], | ||
| "description": "The value used for matching — either an object ID or a name, depending on match_by." |
Contributor
There was a problem hiding this comment.
Schema accepts endpoint value types that current ingest models cannot deserialize.
Line 20/44 now permit non-string value, and Line 91 demonstrates numeric input. But EdgeEndpoint.Value and IngestibleEndpoint.Value are still string in packages/go/ein/incoming_models.go (Line 386) and packages/go/ein/models.go (Line 51). This creates validate-pass but ingest-fail payloads.
💡 Safe contract fix (until typed values are supported end-to-end)
- "value": {
- "type": ["number", "string", "boolean", "array"],
+ "value": {
+ "type": "string",
"description": "The value used for matching — either an object ID or a name, depending on match_by."
},
@@
- "value": {
- "type": ["number", "string", "boolean", "array"],
+ "value": {
+ "type": "string",
"description": "The value used for matching — either an object ID or a name, depending on match_by."
},Also applies to: 44-45, 87-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/go/chow/ingestvalidator/jsonschema/edge.json` around lines 20 - 21,
The JSON schema for endpoint "value" currently permits non-string types which
our ingest models (EdgeEndpoint.Value and IngestibleEndpoint.Value) can't
deserialize; update the schema entries that define the endpoint "value" (where
"type" is currently ["number","string","boolean","array"]) to restrict the type
to "string" only so validation aligns with the Go models, or alternately update
EdgeEndpoint.Value and IngestibleEndpoint.Value in the Go code to accept a typed
representation if you intend to support non-string values end-to-end; ensure the
schema change is applied to all occurrences corresponding to the endpoint value
definitions referenced by EdgeEndpoint.Value and IngestibleEndpoint.Value.
Comment on lines
+56
to
67
| func (s IngestibleEndpoint) IdentityProperty() string { | ||
| switch s.MatchBy { | ||
| case MatchByID: | ||
| return common.ObjectID.String() | ||
| case MatchByName: | ||
| return common.Name.String() | ||
| case MatchByGenericProperty: | ||
| return s.Property | ||
| } | ||
|
|
||
| return "" | ||
| } |
Contributor
There was a problem hiding this comment.
Restore default ID matching in IdentityProperty().
Line 56-67 returns "" when MatchBy is unset. That breaks existing zero-value endpoints that rely on default ID behavior and can emit empty identity property keys.
💡 Proposed fix
func (s IngestibleEndpoint) IdentityProperty() string {
switch s.MatchBy {
- case MatchByID:
+ case "", MatchByID:
return common.ObjectID.String()
case MatchByName:
return common.Name.String()
case MatchByGenericProperty:
return s.Property
}
return ""
}📝 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
| func (s IngestibleEndpoint) IdentityProperty() string { | |
| switch s.MatchBy { | |
| case MatchByID: | |
| return common.ObjectID.String() | |
| case MatchByName: | |
| return common.Name.String() | |
| case MatchByGenericProperty: | |
| return s.Property | |
| } | |
| return "" | |
| } | |
| func (s IngestibleEndpoint) IdentityProperty() string { | |
| switch s.MatchBy { | |
| case "", MatchByID: | |
| return common.ObjectID.String() | |
| case MatchByName: | |
| return common.Name.String() | |
| case MatchByGenericProperty: | |
| return s.Property | |
| } | |
| return "" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/go/ein/models.go` around lines 56 - 67, The IdentityProperty method
on IngestibleEndpoint currently returns an empty string when s.MatchBy is unset;
change its default branch to return the ID property instead of "" so zero-value
endpoints use the default ID behavior. In other words, in the IdentityProperty()
function (handling MatchBy, MatchByID, MatchByName, MatchByGenericProperty),
ensure the fallback/else case returns common.ObjectID.String() (same as
MatchByID) rather than an empty string so identity keys are never emitted empty.
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.
Description
Updates OpenGraph Ingest schema to support a
match_bystrategy of "property" that then allows an OG implementer to specify an arbitrary property to match against for an edge endpoint.Motivation and Context
Resolves BED-7451
HYBRID ENVIRONMENTS PATHS
How Has This Been Tested?
Existing tests.
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes