Conversation
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:
📝 WalkthroughWalkthroughAdds ResolutionError and switches case-insensitive matching from substring to equality; centralizes endpoint resolution into endpoint.ResolveAll; makes errorlist builder pointer-returning and updates Resolver.workerErrors to a pointer; removes IngestUserDataError and reclassifies ingest errors in tasks and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Ingest as Ingest.Process
participant Resolver as endpoint.ResolveAll / Resolver
participant DB as GraphDatabase
participant Builder as errorlist.Builder
Ingest->>Resolver: ResolveAll(batch of endpoint names)
Resolver->>DB: Query matches (case-insensitive equals via toLower)
DB-->>Resolver: Matches / Ambiguity / No match
alt resolution errors
Resolver->>Builder: Add resolution error(s)
end
Resolver-->>Ingest: resolved endpoints + collected errors
Ingest->>Builder: Add submission errors (if any)
Builder-->>Ingest: Build() -> combined error (if any)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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
force-pushed
the
BED-7451
branch
2 times, most recently
from
ec4ab7b to
698ead5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/api/src/services/graphify/tasks.go (1)
172-206:⚠️ Potential issue | 🔴 CriticalVerify: callback error handling conflicts with partial ingest semantics.
The code collects per-file/user-data errors properly, but returning
errs.Build()from the callback creates a transactional conflict. IfBatchOperationrolls back on non-nil callback error, successful writes from successful files are lost—contradicting the partial ingest design. The callback should returnnilto ensure batch commits regardless of per-file errors. Per-file error details should be reported viafileDataarray; only fatal infrastructure errors should escape the callback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/services/graphify/tasks.go` around lines 172 - 206, The callback passed to s.graphdb.BatchOperation should not return errs.Build() unconditionally because that will cause the batch to rollback and lose successful writes; instead ensure per-file errors are recorded in fileData and the callback always returns nil unless a fatal infrastructure error occurs; locate the callback around BatchOperation where ic.BindBatchUpdater is called and processSingleFile is invoked, stop returning errs.Build() from the callback (and do not call errs.Add to trigger transactional rollback), and only propagate non-recoverable infrastructure errors out of the callback while leaving per-file user errors in fileData for reporting.
🤖 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/endpoint/fetch.go`:
- Around line 101-103: The validation branch for equals_ignore_case currently
returns a raw fmt.Errorf; change it to return an endpoint.ResolutionError so
ProcessIngestFile will classify it as user-data (partial-upload) error. Replace
the fmt.Errorf return in the equals_ignore_case type-check (the
matchExpression.Value string type branch) with construction and return of an
endpoint.ResolutionError containing a clear message that the equals_ignore_case
value must be a string and include the actual type (use matchExpression.Value
and %T in the message); ensure you use the endpoint.ResolutionError type or its
constructor used elsewhere in this package so the error is recognized by
ProcessIngestFile.
In `@cmd/api/src/services/graphify/ingestrelationships_integration_test.go`:
- Around line 982-984: The test currently asserts the old sentinel
endpoint.ErrUnresolvable after calling ResolveAll; update it to the new contract
by either asserting the aggregate error string or by type-asserting the returned
err to errorlist.Error and inspecting its Inner/Errors slice for the expected
sub-error(s); specifically modify the assertions around ResolveAll(...) /
endpoint.NewResolver(...) to remove ErrUnresolvable, cast err to errorlist.Error
(or call err.Error() and compare substrings), and update the
require.Len/require.Error checks accordingly (also make the same change for the
duplicate assertions at the other occurrence).
---
Outside diff comments:
In `@cmd/api/src/services/graphify/tasks.go`:
- Around line 172-206: The callback passed to s.graphdb.BatchOperation should
not return errs.Build() unconditionally because that will cause the batch to
rollback and lose successful writes; instead ensure per-file errors are recorded
in fileData and the callback always returns nil unless a fatal infrastructure
error occurs; locate the callback around BatchOperation where
ic.BindBatchUpdater is called and processSingleFile is invoked, stop returning
errs.Build() from the callback (and do not call errs.Add to trigger
transactional rollback), and only propagate non-recoverable infrastructure
errors out of the callback while leaving per-file user errors in fileData for
reporting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b27f7cf5-1260-45c3-a9cb-7e4680f9829f
📒 Files selected for processing (7)
cmd/api/src/services/graphify/endpoint/fetch.gocmd/api/src/services/graphify/endpoint/resolver.gocmd/api/src/services/graphify/ingestrelationships.gocmd/api/src/services/graphify/ingestrelationships_integration_test.gocmd/api/src/services/graphify/models.gocmd/api/src/services/graphify/tasks.gopackages/go/errorlist/error.go
💤 Files with no reviewable changes (1)
- cmd/api/src/services/graphify/models.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/api/src/services/graphify/endpoint/fetch.go (1)
101-103:⚠️ Potential issue | 🟠 MajorWrap
equals_ignore_casetype-validation failures asResolutionError.At Line 101-Line 103, this branch returns raw
fmt.Errorf. That breaks the new resolution-error classification path and can incorrectly promote user-data resolution errors to full ingest failures instead of partial-ingest reporting.🩹 Suggested fix
- if strValue, typeOK := matchExpression.Value.(string); !typeOK { - return nil, fmt.Errorf("case insentitive equals requires value type of string but got %T", matchExpression.Value) + if strValue, typeOK := matchExpression.Value.(string); !typeOK { + return nil, newEndpointMatchErr(fmt.Errorf("case insensitive equals requires value type of string but got %T", matchExpression.Value)) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/services/graphify/endpoint/fetch.go` around lines 101 - 103, The branch that validates matchExpression.Value for the equals_ignore_case operator currently returns a plain fmt.Errorf; replace that with the project’s resolution error type so type-validation failures are classified as ResolutionError (e.g. return a ResolutionError wrapping the same error message) in the equals_ignore_case handling code that checks matchExpression.Value, ensuring the error preserves the descriptive message but uses the ResolutionError constructor/type used elsewhere in the resolver flow.
🤖 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/endpoint/fetch.go`:
- Around line 64-65: The docstring for caseInsensitiveEquals is stale: it says
“contains” although the function builds an equality comparison; update the
comment for caseInsensitiveEquals to state that it constructs a case-insensitive
equality comparison (using toLower on both sides) rather than a containment
check, so the wording accurately reflects the function's semantics.
---
Duplicate comments:
In `@cmd/api/src/services/graphify/endpoint/fetch.go`:
- Around line 101-103: The branch that validates matchExpression.Value for the
equals_ignore_case operator currently returns a plain fmt.Errorf; replace that
with the project’s resolution error type so type-validation failures are
classified as ResolutionError (e.g. return a ResolutionError wrapping the same
error message) in the equals_ignore_case handling code that checks
matchExpression.Value, ensuring the error preserves the descriptive message but
uses the ResolutionError constructor/type used elsewhere in the resolver flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0d7d7559-2193-42d3-a880-b71bbcf8d670
📒 Files selected for processing (7)
cmd/api/src/services/graphify/endpoint/fetch.gocmd/api/src/services/graphify/endpoint/resolver.gocmd/api/src/services/graphify/ingestrelationships.gocmd/api/src/services/graphify/ingestrelationships_integration_test.gocmd/api/src/services/graphify/models.gocmd/api/src/services/graphify/tasks.gopackages/go/errorlist/error.go
💤 Files with no reviewable changes (1)
- cmd/api/src/services/graphify/models.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/services/graphify/tasks.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cmd/api/src/services/graphify/endpoint/fetch.go (2)
64-72:⚠️ Potential issue | 🟡 MinorFix stale docstring: function performs equality, not containment.
The docstring on line 65 still references "contains" behavior, but the implementation uses
cypher.OperatorEqualsfor equality comparison. This is misleading.📝 Suggested fix
-// caseInsensitiveEquals constructs a Cypher comparison that checks if the reference -// property contains the target value in a case-insensitive manner using the `toLower` function. +// caseInsensitiveEquals constructs a Cypher comparison that checks if the reference +// property equals the target value in a case-insensitive manner using the `toLower` function. func caseInsensitiveEquals(reference, target graph.Criteria) *cypher.Comparison {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/services/graphify/endpoint/fetch.go` around lines 64 - 72, The docstring for function caseInsensitiveEquals is stale (mentions "contains") while the implementation uses cypher.OperatorEquals for equality; update the comment above caseInsensitiveEquals to state it constructs a case-insensitive equality comparison using toLower rather than a containment check, and ensure the wording references the function name caseInsensitiveEquals and the use of cypher.ToLowerFunction and cypher.OperatorEquals so it's clear what the function does.
100-110:⚠️ Potential issue | 🟠 MajorReturn
ResolutionErrorand fix typo for consistency.Two issues in this branch:
Inconsistent error type: Other error paths in this file (lines 184, 186, 212, 214) wrap errors with
NewResolutionError, but this validation failure returns a rawfmt.Errorf. Per the PR's error handling design,ProcessIngestFilepromotes onlyendpoint.ResolutionErrorvalues intoUserDataErrs, so this error will be misclassified.Typo: "insentitive" should be "insensitive".
🐛 Proposed fix
case ein.OperatorEqualsIgnoreCase: if strValue, typeOK := matchExpression.Value.(string); !typeOK { - return nil, fmt.Errorf("case insentitive equals requires value type of string but got %T", matchExpression.Value) + return nil, NewResolutionError(fmt.Errorf("case insensitive equals requires value type of string but got %T", matchExpression.Value)) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/services/graphify/endpoint/fetch.go` around lines 100 - 110, The case handling for ein.OperatorEqualsIgnoreCase returns a plain fmt.Errorf with a typo; change it to return endpoint.NewResolutionError(...) (matching other branches that wrap validation errors) and correct the message text from "insentitive" to "insensitive"; locate the branch inside the switch handling matchExpression (case ein.OperatorEqualsIgnoreCase) and replace the fmt.Errorf return with a call to endpoint.NewResolutionError that includes the formatted type mismatch message referencing matchExpression.Value so the error is treated as a ResolutionError by ProcessIngestFile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/api/src/services/graphify/endpoint/fetch.go`:
- Around line 64-72: The docstring for function caseInsensitiveEquals is stale
(mentions "contains") while the implementation uses cypher.OperatorEquals for
equality; update the comment above caseInsensitiveEquals to state it constructs
a case-insensitive equality comparison using toLower rather than a containment
check, and ensure the wording references the function name caseInsensitiveEquals
and the use of cypher.ToLowerFunction and cypher.OperatorEquals so it's clear
what the function does.
- Around line 100-110: The case handling for ein.OperatorEqualsIgnoreCase
returns a plain fmt.Errorf with a typo; change it to return
endpoint.NewResolutionError(...) (matching other branches that wrap validation
errors) and correct the message text from "insentitive" to "insensitive"; locate
the branch inside the switch handling matchExpression (case
ein.OperatorEqualsIgnoreCase) and replace the fmt.Errorf return with a call to
endpoint.NewResolutionError that includes the formatted type mismatch message
referencing matchExpression.Value so the error is treated as a ResolutionError
by ProcessIngestFile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 155c87a4-1939-4915-8aca-db56522f76f6
📒 Files selected for processing (7)
cmd/api/src/services/graphify/endpoint/fetch.gocmd/api/src/services/graphify/endpoint/resolver.gocmd/api/src/services/graphify/ingestrelationships.gocmd/api/src/services/graphify/ingestrelationships_integration_test.gocmd/api/src/services/graphify/models.gocmd/api/src/services/graphify/tasks.gopackages/go/errorlist/error.go
💤 Files with no reviewable changes (1)
- cmd/api/src/services/graphify/models.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/services/graphify/endpoint/resolver.go
- cmd/api/src/services/graphify/tasks.go
craigw-SO
reviewed
Mar 11, 2026
Contributor
craigw-SO
left a comment
craigw-SO
left a comment
There was a problem hiding this comment.
Just a minor typo, but one that's exposed as part of an error message.
zinic
force-pushed
the
BED-7451
branch
2 times, most recently
from
c1c04ec to
81474b4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/endpoint/fetch.go`:
- Around line 185-186: getNodeObjectID can return both operational DB errors and
true resolution misses; change the two branches that currently do "if objectID,
err := getNodeObjectID(...); err != nil { return ingestEntry,
NewResolutionError(err) }" so they only wrap known resolution-miss errors (e.g.,
ErrNoMatch, ErrAmbiguous or whatever sentinel errors getNodeObjectID returns)
with NewResolutionError; for any other error (detected via errors.Is or a helper
like isResolutionError(err)), propagate the original err unchanged. Update both
occurrences (the branch at the shown diff and the similar branch around lines
213-214) to perform this conditional wrapping so DB/operational errors are not
misclassified as ResolutionError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 29d30f86-2450-4111-8d09-aea313b06925
📒 Files selected for processing (7)
cmd/api/src/services/graphify/endpoint/fetch.gocmd/api/src/services/graphify/endpoint/resolver.gocmd/api/src/services/graphify/ingestrelationships.gocmd/api/src/services/graphify/ingestrelationships_integration_test.gocmd/api/src/services/graphify/models.gocmd/api/src/services/graphify/tasks.gopackages/go/errorlist/error.go
💤 Files with no reviewable changes (1)
- cmd/api/src/services/graphify/models.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/services/graphify/ingestrelationships_integration_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/api/src/services/graphify/ingestrelationships_integration_test.go (1)
982-984: Consider usingrequire.ErrorContainsfor more specific assertions.The test uses
require.Error(t, err)which validates an error occurred, but doesn't verify the error type/message. Usingrequire.ErrorContains(t, err, "unable to resolve endpoint")would provide stronger validation that the correct error path was triggered.💡 Optional improvement
updatedIngestibleRels, err := endpoint.ResolveAll(testContext.Context(), endpoint.NewResolver(testContext.Graph.Database), rels) - require.Error(t, err) + require.ErrorContains(t, err, "unable to resolve endpoint") require.Len(t, updatedIngestibleRels, 0)🤖 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` around lines 982 - 984, Replace the broad require.Error assertion with a more specific check using require.ErrorContains to assert the expected error message; in the test around ResolveAll / endpoint.NewResolver (variables updatedIngestibleRels, err), change require.Error(t, err) to require.ErrorContains(t, err, "unable to resolve endpoint") and keep the require.Len(t, updatedIngestibleRels, 0) assertion to ensure the correct error path was exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/api/src/services/graphify/ingestrelationships_integration_test.go`:
- Around line 982-984: Replace the broad require.Error assertion with a more
specific check using require.ErrorContains to assert the expected error message;
in the test around ResolveAll / endpoint.NewResolver (variables
updatedIngestibleRels, err), change require.Error(t, err) to
require.ErrorContains(t, err, "unable to resolve endpoint") and keep the
require.Len(t, updatedIngestibleRels, 0) assertion to ensure the correct error
path was exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d53c0b80-e58a-43ed-9716-9e5ad971d7ae
📒 Files selected for processing (7)
cmd/api/src/services/graphify/endpoint/fetch.gocmd/api/src/services/graphify/endpoint/resolver.gocmd/api/src/services/graphify/ingestrelationships.gocmd/api/src/services/graphify/ingestrelationships_integration_test.gocmd/api/src/services/graphify/models.gocmd/api/src/services/graphify/tasks.gopackages/go/errorlist/error.go
💤 Files with no reviewable changes (1)
- cmd/api/src/services/graphify/models.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/services/graphify/endpoint/resolver.go
- packages/go/errorlist/error.go
…ng for resolution failure constitues as a partial upload - BED-7451
LawsonWillard
approved these changes
Mar 12, 2026
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.
Description
Motivation and Context
Resolves BED-7451
How Has This Been Tested?
Updated test cases to cover the particular failures.
Types of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Tests