ingest yaml with filename #1630
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis PR extends the enqueue DAG run endpoint to accept either an inline YAML specification or a filename reference pointing to an existing YAML file. The request body is modified with a new optional filename field and a oneOf constraint, while backend logic handles reading and validating YAML files from disk when a filename is provided. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Handler as API Handler
participant FileSystem as File System
participant Config as Base Config
participant DAGLoader as DAG Loader
Client->>Handler: POST /dags/{fileName}/enqueue<br/>(with Filename)
Handler->>Handler: Validate: not both spec & filename
Handler->>Config: Read base config
Config-->>Handler: Config data
Handler->>Handler: Resolve ALT_DAGS_DIR from env
Handler->>FileSystem: Read YAML file<br/>(check .yaml/.yml extension)
FileSystem-->>Handler: File content (≤1 MiB)
Handler->>DAGLoader: Load DAG from file content
DAGLoader-->>Handler: DAG object
Handler-->>Client: 200 OK (DAG run enqueued)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/frontend/api/v2/dagruns.go (1)
261-265:⚠️ Potential issue | 🟡 MinorAudit metadata should reflect filename-based enqueue.
inlineis alwaystrue, even when a filename is used, which makes audit logs inaccurate.🧾 Suggested audit flag fix
- detailsMap := map[string]any{ - "dag_name": dag.Name, - "dag_run_id": dagRunId, - "inline": true, - } + inline := request.Body.Spec != nil + detailsMap := map[string]any{ + "dag_name": dag.Name, + "dag_run_id": dagRunId, + "inline": inline, + } + if request.Body.Filename != nil { + detailsMap["filename"] = *request.Body.Filename + }
🤖 Fix all issues with AI agents
In `@internal/service/frontend/api/v2/dagruns.go`:
- Around line 161-166: Update the user-facing error message so it references the
actual API field name `filename` instead of “url”: change the Error returned in
the branch that checks request.Body.Spec and request.Body.Filename (the
conditional using request.Body.Spec == nil && request.Body.Filename == nil ||
request.Body.Spec != nil && request.Body.Filename != nil) to set Message to
something like "either give spec or filename; don't give both" so clients see
the correct field name.
- Around line 193-212: The current code unsafely assumes
obj["env"].([]any)[0].(string) and will panic or miss ALT_DAGS_DIR; change the
logic in the block after yaml.Unmarshal by: validate that obj["env"] exists and
is a []any, iterate over each element, assert each entry is a string, look for a
string that starts with "ALT_DAGS_DIR=" (or split on '=' and compare key),
extract the value into altDagDir when found, and only then trim the prefix and
rebuild filename; if no ALT_DAGS_DIR is found return the existing Error
response; keep using the same variables (obj, altDagDir, filename) and preserve
the yaml.Unmarshal error handling.
🧹 Nitpick comments (1)
api/v2/api.yaml (1)
1614-1622: Constrainfilenameto allowed basename + extension.The backend only accepts
.yaml/.yml; the schema currently allows any string. Adding a pattern reduces invalid requests and clarifies the contract.📄 Suggested schema constraint
filename: type: string description: "Filename for the yaml file" + pattern: "^[a-zA-Z0-9._-]+\\.(ya?ml)$"
| if request.Body.Spec == nil && request.Body.Filename == nil || request.Body.Spec != nil && request.Body.Filename != nil{ | ||
| return nil, &Error{ | ||
| HTTPStatus: http.StatusBadRequest, | ||
| Code: api.ErrorCodeBadRequest, | ||
| Message: "either give spec or url; don't give both", | ||
| } |
There was a problem hiding this comment.
Fix user-facing error text to match the field name.
The error says “url” but the API uses filename, which is confusing for clients.
✏️ Suggested text fix
- Message: "either give spec or url; don't give both",
+ Message: "either give spec or filename; don't give both",📝 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.
| if request.Body.Spec == nil && request.Body.Filename == nil || request.Body.Spec != nil && request.Body.Filename != nil{ | |
| return nil, &Error{ | |
| HTTPStatus: http.StatusBadRequest, | |
| Code: api.ErrorCodeBadRequest, | |
| Message: "either give spec or url; don't give both", | |
| } | |
| if request.Body.Spec == nil && request.Body.Filename == nil || request.Body.Spec != nil && request.Body.Filename != nil{ | |
| return nil, &Error{ | |
| HTTPStatus: http.StatusBadRequest, | |
| Code: api.ErrorCodeBadRequest, | |
| Message: "either give spec or filename; don't give both", | |
| } |
🤖 Prompt for AI Agents
In `@internal/service/frontend/api/v2/dagruns.go` around lines 161 - 166, Update
the user-facing error message so it references the actual API field name
`filename` instead of “url”: change the Error returned in the branch that checks
request.Body.Spec and request.Body.Filename (the conditional using
request.Body.Spec == nil && request.Body.Filename == nil || request.Body.Spec !=
nil && request.Body.Filename != nil) to set Message to something like "either
give spec or filename; don't give both" so clients see the correct field name.
| obj := make(map[string]any) | ||
| err = yaml.Unmarshal(baseData,&obj) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error unmarshalling base config: %w", err) | ||
| } | ||
|
|
||
| altDagDir := obj["env"].([]any)[0].(string) | ||
| fmt.Println("ALDAGDIR",altDagDir) | ||
|
|
||
| if strings.HasPrefix(altDagDir, "ALT_DAGS_DIR=") { | ||
| altDagDir = strings.TrimPrefix(altDagDir, "ALT_DAGS_DIR=") | ||
| filename = fmt.Sprintf("%s/%s", altDagDir, filepath.Base(filename)) | ||
| } else { | ||
| return nil, &Error{ | ||
| HTTPStatus: http.StatusBadRequest, | ||
| Code: api.ErrorCodeBadRequest, | ||
| Message: "Make sure ALT_DAGS_DIR is set in the config file", | ||
| } | ||
| } | ||
| const maxSpecBytes = 1 << 20 // 1 MiB (tune or reuse config) |
There was a problem hiding this comment.
Guard ALT_DAGS_DIR extraction to avoid panics and missed configs.
obj["env"].([]any)[0].(string) will panic if env is missing/empty/not a slice, and it only inspects the first entry (so ALT_DAGS_DIR can be missed). Parse the env list safely and search for the key before constructing the path.
🔧 Safer env parsing and ALT_DAGS_DIR lookup
- obj := make(map[string]any)
- err = yaml.Unmarshal(baseData,&obj)
+ obj := make(map[string]any)
+ err = yaml.Unmarshal(baseData, &obj)
if err != nil {
return nil, fmt.Errorf("error unmarshalling base config: %w", err)
}
-
- altDagDir := obj["env"].([]any)[0].(string)
- fmt.Println("ALDAGDIR",altDagDir)
-
- if strings.HasPrefix(altDagDir, "ALT_DAGS_DIR=") {
- altDagDir = strings.TrimPrefix(altDagDir, "ALT_DAGS_DIR=")
- filename = fmt.Sprintf("%s/%s", altDagDir, filepath.Base(filename))
- } else {
- return nil, &Error{
- HTTPStatus: http.StatusBadRequest,
- Code: api.ErrorCodeBadRequest,
- Message: "Make sure ALT_DAGS_DIR is set in the config file",
- }
- }
+ envRaw, ok := obj["env"].([]any)
+ if !ok || len(envRaw) == 0 {
+ return nil, &Error{
+ HTTPStatus: http.StatusBadRequest,
+ Code: api.ErrorCodeBadRequest,
+ Message: "Make sure ALT_DAGS_DIR is set in the config file",
+ }
+ }
+ var altDagDir string
+ for _, v := range envRaw {
+ if s, ok := v.(string); ok && strings.HasPrefix(s, "ALT_DAGS_DIR=") {
+ altDagDir = strings.TrimPrefix(s, "ALT_DAGS_DIR=")
+ break
+ }
+ }
+ if altDagDir == "" {
+ return nil, &Error{
+ HTTPStatus: http.StatusBadRequest,
+ Code: api.ErrorCodeBadRequest,
+ Message: "Make sure ALT_DAGS_DIR is set in the config file",
+ }
+ }
+ filename = filepath.Join(altDagDir, filepath.Base(filename))🤖 Prompt for AI Agents
In `@internal/service/frontend/api/v2/dagruns.go` around lines 193 - 212, The
current code unsafely assumes obj["env"].([]any)[0].(string) and will panic or
miss ALT_DAGS_DIR; change the logic in the block after yaml.Unmarshal by:
validate that obj["env"] exists and is a []any, iterate over each element,
assert each entry is a string, look for a string that starts with
"ALT_DAGS_DIR=" (or split on '=' and compare key), extract the value into
altDagDir when found, and only then trim the prefix and rebuild filename; if no
ALT_DAGS_DIR is found return the existing Error response; keep using the same
variables (obj, altDagDir, filename) and preserve the yaml.Unmarshal error
handling.
| altDagDir := obj["env"].([]any)[0].(string) | ||
| fmt.Println("ALDAGDIR",altDagDir) | ||
|
|
||
| if strings.HasPrefix(altDagDir, "ALT_DAGS_DIR=") { |
There was a problem hiding this comment.
I'm thinking we should use ALT_DAGS_DIR to be set in SearchPaths field of DAG store.
dagu/internal/persis/filedag/store.go
Line 34 in e05708d
The fix would be adding
ALT_DAGS_DIR to config package, update docs for the new environment variable, and adding ALT_DAGS_DIR value to the SearchPaths. In this case there's no need to fix this API. What do you think?
There was a problem hiding this comment.
Yes that will be a much cleaner and systematic approach. Will work on that
|
for this, config.yaml will have one more variable altDagDir under paths: which will be used for locating the alternate dags |
|
@sahalisro-blip Yes, I think that's correct. |
| |---------------------|---------|-------------| | ||
| | `DAGU_HOME` | - | Base directory that overrides all path configurations | | ||
| | `DAGU_DAGS_DIR` | `~/.config/dagu/dags` | Directory for DAG definitions | | ||
| | `DAGU_ALT_DAGS_DIR` | - | Additional directory to search for DAG definitions | |
There was a problem hiding this comment.
Please add default path
~/.config/dagu/alt_dags
4 participants
now dags can only be ingest with the name.
user has to define ALT_DAGS_DIR variable in base.yaml
Spec is still working. #1609
Summary by CodeRabbit