lisa/feat/workflow-import-route#413
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughDelegates CLI workflow seeding to a new shared relational importer, adds a multipart POST /workflows/import HTTP handler with per-file and body size limits, updates OpenAPI docs/schemas, and adds unit and integration tests for the service and handler. ChangesWorkflow Seed Import Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPHandler as WorkflowImportHandler
participant Decoder as DecodeSeedDefinitions
participant Service as ImportSeedDefinitions
participant DB as Database
Client->>HTTPHandler: POST /workflows/import (multipart files)
HTTPHandler->>HTTPHandler: validate file count & sizes
loop per file
HTTPHandler->>Decoder: decode JSON (limited reader)
Decoder->>Service: send decoded definitions
Service->>DB: transactional upserts (deterministic IDs)
Service->>HTTPHandler: per-file SeedSummary
end
HTTPHandler->>Client: aggregated WorkflowImportDataResponse (200/400/413)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
gusfcarvalho
reviewed
Jun 3, 2026
Contributor
gusfcarvalho
left a comment
gusfcarvalho
left a comment
There was a problem hiding this comment.
Findings inline (5 threads). No blockers — all Low/nit. Clean, well-tested PR; the seed-engine de-duplication and the extra resource hardening on this endpoint (per-file + file-count + BodyLimit caps, admin-only) are nice. Build/vet/gofmt/unit tests pass locally; the previously self-reviewed BodyLimit sizing bug is genuinely fixed and covered by a route-level test.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/docs.go`:
- Around line 25286-25293: The OpenAPI entry for the request parameter "files"
incorrectly models a single file while WorkflowImportHandler.Import (method
Import) actually reads ctx.MultipartForm() and handles multiple files; update
the Swagger/Swagger-annotated parameter for "files" to represent a multi-file
form field by changing its schema to an array of files (e.g., type: "array",
items: { "type": "file" }, and use collectionFormat: "multi" or the equivalent
for the generator in use) so clients understand multiple files under the "files"
field are accepted; locate the parameter definition for "files" in the docs (the
parameter with name "files", in "formData") and replace the single-file spec
with the array-of-file spec.
In `@docs/swagger.json`:
- Around line 40686-40717: Update the workflows.SeedSummary OpenAPI schema to
declare which counters are always returned by adding a required array (e.g.,
include
"control_relationships","definitions_created","definitions_updated","dependencies","failed","instances","role_assignments","skipped","steps")
and add a description string for each property in the properties object
explaining what that counter represents (for example, describe what
"definitions_created" and "definitions_updated" count and what "failed"
indicates). Ensure the required array references the exact property names in
workflows.SeedSummary and keep descriptions concise and consumer-facing to
improve the API contract clarity.
- Around line 25280-25287: Update the Swagger parameter for "files" (parameter
name "files", type "file", in "formData") to explicitly state that multiple
files are supported by repeating the "files" form field in a multipart/form-data
request (i.e., send multiple parts with the same name), so API consumers know to
upload one or more files by repeating the "files" parameter rather than trying a
single multi-file value.
- Around line 41334-41381: Update the three OpenAPI definitions
workflows.WorkflowImportDataResponse, workflows.WorkflowImportFileResult, and
workflows.WorkflowImportResponse to include "required" arrays and add property
"description" strings for each property; specifically mark
WorkflowImportDataResponse.data as required, mark
WorkflowImportFileResult.filename and WorkflowImportFileResult.success as
required, and mark WorkflowImportResponse.total_files,
WorkflowImportResponse.successful_files, WorkflowImportResponse.failed_files,
and WorkflowImportResponse.results as required, and add clear descriptions for
each property (e.g., what filename, message, success, summary, total_files,
successful_files, failed_files, results and data represent and whether they can
be null) so consumers know guaranteed fields and semantics.
In `@docs/swagger.yaml`:
- Around line 9391-9421: The new OpenAPI schemas
(workflows.WorkflowImportFileResult, workflows.WorkflowImportResponse, and
workflows.WorkflowImportDataResponse) lack required arrays so every property is
treated as optional; add explicit "required" arrays to each schema listing the
always-present fields (e.g., for workflows.WorkflowImportFileResult include
"filename" and "success", for workflows.WorkflowImportResponse include
"total_files", "successful_files", "failed_files" and likely "results" and
"summary" as applicable, and for workflows.WorkflowImportDataResponse include
"data") to make the contract explicit for consumers.
- Around line 26046-26051: The Swagger spec currently models the form-data
parameter "files" as a single type: file which only supports one file; update
the parameter "files" in docs/swagger.yaml to represent multiple files by
changing it to an array formData parameter (e.g., type: array, items: { type:
string, format: binary } and collectionFormat: multi) so the endpoint that
accepts "one or more seed JSON files" is accurately described; locate the
parameter named "files" in the parameters block and replace its single-file
definition with the array/collectionFormat workaround (or alternatively note
migration to OpenAPI 3 for native multipart file arrays).
In `@internal/api/handler/workflows/import.go`:
- Around line 73-113: After calling ctx.MultipartForm() in
WorkflowImportHandler.Import, ensure the multipart temporary files are removed
by deferring form.RemoveAll(); specifically, once form, err :=
ctx.MultipartForm() succeeds, add a defer that checks form != nil and calls
form.RemoveAll() (discarding any error) so temp files on disk are cleaned up
after processing; reference the Import method on WorkflowImportHandler and the
form variable returned by ctx.MultipartForm().
In `@internal/service/relational/workflows/seedimport.go`:
- Around line 84-91: The DecodeSeedDefinitions function currently allows
trailing JSON after the array because json.Decoder.Decode stops at the first
value; after successfully decoding into definitions, try to decode one more
value into a temporary variable (e.g., var extra interface{}) and if that Decode
does not return io.EOF, return an error indicating unexpected trailing JSON;
update DecodeSeedDefinitions to perform this extra decode check so malformed
inputs like `[...]{...}` are rejected.
- Around line 316-331: The loop over seedDef.Instances builds a deterministic ID
with deterministicSeedUUID("workflow-instance", seedDef.Key, seedInstance.Name)
without checking for duplicate instance names, causing upserts to collide;
before deriving instanceID (i.e., before calling deterministicSeedUUID) add a
guard like importSeedSteps does: maintain a local map[string]struct{} of seen
instance names for this seedDef, check seedInstance.Name against it, and return
an error if duplicated so duplicate names are rejected rather than merged.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: ab174d22-0c17-4d7e-8038-2779b5f0ba99
📒 Files selected for processing (11)
cmd/seed/workflows.gocmd/seed/workflows_test.godocs/docs.godocs/swagger.jsondocs/swagger.yamlinternal/api/handler/api.gointernal/api/handler/workflows/import.gointernal/api/handler/workflows/import_integration_test.gointernal/api/handler/workflows/import_test.gointernal/service/relational/workflows/seedimport.gointernal/service/relational/workflows/seedimport_test.go
gusfcarvalho
approved these changes
Jun 3, 2026
Contributor
gusfcarvalho
left a comment
gusfcarvalho
left a comment
There was a problem hiding this comment.
Approving. All substantive findings are resolved and verified at 8cdcbfe.
My 5 original findings — 4 fixed (omitempty→json:"summary"; 413 propagation for chunked over-limit bodies + test; defer form.RemoveAll() for multipart temp files; full JWT+admin+BodyLimit middleware-chain test), 1 was informational (documented continue-on-error contract).
CodeRabbit's 2 real source findings — both fixed with tests: trailing-JSON rejection in DecodeSeedDefinitions (+TestDecodeSeedDefinitionsRejectsTrailingJSON) and the duplicate-instance-name guard mirroring importSeedSteps (+TestImportWorkflowSeedDefinitionRejectsDuplicateInstanceNames, asserting rollback). Multi-file files param doc clarified and swagger regenerated.
Local build / go vet / gofmt / unit tests clean; CI green on check-diff, lint, unit-tests, and integration-tests.
The 3 remaining open threads are 🔵 Trivial swaggo-generated-schema required/description nits — non-blocking, fine to defer or resolve as won't-fix. Nice refactor consolidating the seed engine; solid test coverage.
Contributor
Author
|
PR approved. Marking as ready for e2e. |
gusfcarvalho
requested changes
Jun 3, 2026
Contributor
gusfcarvalho
left a comment
gusfcarvalho
left a comment
There was a problem hiding this comment.
Tested the import end-to-end against the running stack. One blocking issue (re-import/reseed of a previously soft-deleted definition fails — details inline) plus a result-message accuracy nit. See inline threads.
gusfcarvalho
approved these changes
Jun 4, 2026
Contributor
gusfcarvalho
left a comment
gusfcarvalho
left a comment
There was a problem hiding this comment.
Approving. The soft-delete re-import failure and the misleading import-count message are both fixed and verified (resurrection regression tests pass locally; lint/unit/check-diff CI green). Two non-blocking Low observations left inline (instance schedule can stay NULL on re-import; upsert-only import doesn't prune removed children) — surface them for a future pass, not required here.
Contributor
Author
|
PR approved. Marking as ready for e2e. |
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.
automated implementation by lisa.
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes / Validation