fix: return DAG with errors instead of 404 for invalid YAML files#1975
fix: return DAG with errors instead of 404 for invalid YAML files#1975
Conversation
The GetDAGDetails API returned 404 for DAG files that exist but have YAML parse errors. Two issues caused this: 1. loadDAG had inconsistent BuildFlagAllowBuildErrors coverage — only the loadDAGsFromFile error path checked the flag, while base config errors, empty files, and sub-DAG naming errors bypassed it. Added an errorDAG helper to handle all error paths consistently, matching the existing LoadYAMLWithOpts behavior. 2. GetDAGDetails mapped every error to 404 NotFound. Now it returns 404 only when the file doesn't exist (os.ErrNotExist) and 500 for other failures. The error wrapping in getDAGDetailsData preserves the original error chain.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR refactors error handling in the DAG loader by introducing a helper function to centralize Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/spec/loader.go`:
- Around line 319-325: When synthesizing the error DAG inside the errorDAG
closure, preserve an explicitly provided DAG name from the loader options
instead of always using defaultName(filePath); update the Name field assignment
in errorDAG to use ctx.opts.Name (or the options' Name property) when non-empty,
falling back to defaultName(filePath) only if no override is present so
spec.Load(..., spec.WithName("override"), ...) retains the explicit name on
file-based failure paths.
In `@internal/service/frontend/api/v1/dags_internal_test.go`:
- Around line 155-160: The test currently only checks for a non-nil error from
api.GetDAGDetails; instead assert the error is a *localapi.Error and that its
HTTPStatus (and/or Code) equals 404 to enforce the 404 contract. Update the
assertion after calling api.GetDAGDetails to cast or use errors.As to a
*localapi.Error (referencing localapi.Error) and then require.Equal on
err.HTTPStatus (and optionally err.Code) to 404; keep the existing call to
openapi.GetDAGDetailsRequestObject and require helpers (e.g.,
require.True/require.Equal or require.IsType) to make the check explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54a25892-4f20-457b-a5ff-1d0bd07d7ec0
📒 Files selected for processing (4)
internal/core/spec/loader.gointernal/core/spec/loader_test.gointernal/service/frontend/api/v1/dags.gointernal/service/frontend/api/v1/dags_internal_test.go
| errorDAG := func(err error) (*core.DAG, error) { | ||
| if ctx.opts.Has(BuildFlagAllowBuildErrors) { | ||
| return &core.DAG{ | ||
| Name: defaultName(filePath), | ||
| Location: filePath, | ||
| SourceFile: filePath, | ||
| BuildErrors: []error{err}, |
There was a problem hiding this comment.
Preserve WithName when synthesizing an error DAG.
Line 322 always falls back to defaultName(filePath), so spec.Load(..., spec.WithName("override"), spec.WithAllowBuildErrors()) loses the explicit DAG name only on file-based failure paths. LoadYAMLWithOpts keeps opts.Name, so this makes the two loaders inconsistent.
Suggested tweak
errorDAG := func(err error) (*core.DAG, error) {
if ctx.opts.Has(BuildFlagAllowBuildErrors) {
+ name := ctx.opts.Name
+ if name == "" {
+ name = defaultName(filePath)
+ }
return &core.DAG{
- Name: defaultName(filePath),
+ Name: name,
Location: filePath,
SourceFile: filePath,
BuildErrors: []error{err},
}, nil
}📝 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.
| errorDAG := func(err error) (*core.DAG, error) { | |
| if ctx.opts.Has(BuildFlagAllowBuildErrors) { | |
| return &core.DAG{ | |
| Name: defaultName(filePath), | |
| Location: filePath, | |
| SourceFile: filePath, | |
| BuildErrors: []error{err}, | |
| errorDAG := func(err error) (*core.DAG, error) { | |
| if ctx.opts.Has(BuildFlagAllowBuildErrors) { | |
| name := ctx.opts.Name | |
| if name == "" { | |
| name = defaultName(filePath) | |
| } | |
| return &core.DAG{ | |
| Name: name, | |
| Location: filePath, | |
| SourceFile: filePath, | |
| BuildErrors: []error{err}, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/spec/loader.go` around lines 319 - 325, When synthesizing the
error DAG inside the errorDAG closure, preserve an explicitly provided DAG name
from the loader options instead of always using defaultName(filePath); update
the Name field assignment in errorDAG to use ctx.opts.Name (or the options' Name
property) when non-empty, falling back to defaultName(filePath) only if no
override is present so spec.Load(..., spec.WithName("override"), ...) retains
the explicit name on file-based failure paths.
| _, err := api.GetDAGDetails(context.Background(), openapi.GetDAGDetailsRequestObject{ | ||
| FileName: "does-not-exist", | ||
| }) | ||
| // Should return an error (which becomes a 404) | ||
| require.Error(t, err) | ||
| } |
There was a problem hiding this comment.
Assert the 404 contract explicitly.
Line 159 only checks that an error exists, so this still passes if GetDAGDetails regresses back to a 500. Assert *localapi.Error and its HTTPStatus/Code instead.
Suggested assertion
_, err := api.GetDAGDetails(context.Background(), openapi.GetDAGDetailsRequestObject{
FileName: "does-not-exist",
})
- // Should return an error (which becomes a 404)
- require.Error(t, err)
+ var apiErr *localapi.Error
+ require.ErrorAs(t, err, &apiErr)
+ require.Equal(t, 404, apiErr.HTTPStatus)
+ require.Equal(t, openapi.ErrorCodeNotFound, apiErr.Code)📝 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.
| _, err := api.GetDAGDetails(context.Background(), openapi.GetDAGDetailsRequestObject{ | |
| FileName: "does-not-exist", | |
| }) | |
| // Should return an error (which becomes a 404) | |
| require.Error(t, err) | |
| } | |
| _, err := api.GetDAGDetails(context.Background(), openapi.GetDAGDetailsRequestObject{ | |
| FileName: "does-not-exist", | |
| }) | |
| var apiErr *localapi.Error | |
| require.ErrorAs(t, err, &apiErr) | |
| require.Equal(t, 404, apiErr.HTTPStatus) | |
| require.Equal(t, openapi.ErrorCodeNotFound, apiErr.Code) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/frontend/api/v1/dags_internal_test.go` around lines 155 -
160, The test currently only checks for a non-nil error from api.GetDAGDetails;
instead assert the error is a *localapi.Error and that its HTTPStatus (and/or
Code) equals 404 to enforce the 404 contract. Update the assertion after calling
api.GetDAGDetails to cast or use errors.As to a *localapi.Error (referencing
localapi.Error) and then require.Equal on err.HTTPStatus (and optionally
err.Code) to 404; keep the existing call to openapi.GetDAGDetailsRequestObject
and require helpers (e.g., require.True/require.Equal or require.IsType) to make
the check explicit.
- Preserve explicit WithName option in errorDAG closure, falling back to defaultName(filePath) only when no override is set, matching LoadYAMLWithOpts behavior. - Assert 404 contract explicitly in TestGetDAGDetails_NonExistent_Returns404 by checking HTTPStatus and ErrorCode on the returned *Error.
1 participant
Summary
loadDAGnow checksBuildFlagAllowBuildErrorsat all error paths (base config, empty file, sub-DAG naming), not justloadDAGsFromFile. Uses anerrorDAGhelper to match the existingLoadYAMLWithOptspattern.GetDAGDetailsreturns 404 only foros.ErrNotExist(file truly missing), not for parse/build errors. Other errors return 500.getDAGDetailsDatawraps the original error with%winstead of replacing it with a misleading "not found" message.Test plan
WithAllowBuildErrors_YAMLSyntaxErrorandWithAllowBuildErrors_EmptyFileverifyspec.Loadreturns a DAG withBuildErrorsfor broken/empty YAMLTestGetDAGDetails_InvalidYAML_Returns200WithErrorsverifies the API returns 200 with errors for an invalid YAML fileTestGetDAGDetails_NonExistent_Returns404verifies missing files still return errorscore/spec,persis/filedag,service/frontend/api/v1,cmd)Summary by CodeRabbit
Release Notes
Bug Fixes
Tests