feat(server): add ProjectImportStatus[VIZ-1641] #1614
Conversation
✅ Deploy Preview for reearth-web canceled.
|
WalkthroughThis change removes the GraphQL mutation-based project import functionality and its associated test, replacing it with a new project-level import status field and enum. The chunked file upload process is refactored to create a project at the start, track import status, and update it asynchronously as the import progresses, with status persisted in the database. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server (UploadManager)
participant DB
participant Importer
Client->>Server (UploadManager): POST /api/split-import (chunk 0)
Server (UploadManager)->>DB: Create new Project (importStatus: PROCESSING)
Server (UploadManager)->>Client: Respond with ProjectID
loop For each chunk
Client->>Server (UploadManager): POST /api/split-import (chunk N, ProjectID)
Server (UploadManager)->>DB: Update UploadSession with ProjectID
end
Server (UploadManager)->>Importer: On last chunk, assemble file & start async import
Importer->>DB: Import data into Project (using ProjectID)
alt Import fails
Importer->>DB: Update Project importStatus to FAILED
else Import succeeds
Importer->>DB: Update Project importStatus to SUCCESS
end
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
hexaforce
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (3)
server/e2e/gql_project_export_import_test.go (1)
143-173:⚠️ Potential issueUpdate test to match the new import status API
The test is failing because it's still using the removed
importProjectGraphQL mutation. According to the pipeline failure, the GraphQL API now returns an error: "Cannot query field 'importProject' on type 'Mutation'". This aligns with the PR objectives which state that theimportProjectmutation was removed and replaced with animportStatusfield on the Project type.You need to update this test to work with the new import API approach that uses status tracking instead of a direct mutation. Consider:
func importProject(t *testing.T, e *httpexpect.Expect, filePath string) *httpexpect.Value { file, err := os.Open(filePath) if err != nil { t.Fatalf("failed to open file: %v", err) } defer func() { if cerr := file.Close(); cerr != nil && err == nil { err = cerr } }() requestBody := map[string]interface{}{ - "operationName": "ImportProject", + "operationName": "UploadProjectFile", "variables": map[string]interface{}{ "teamId": wID.String(), "file": nil, }, - "query": `mutation ImportProject($teamId: ID!, $file: Upload!) { - importProject(input: {teamId: $teamId, file: $file}) { - projectData - __typename - } - }`, + "query": `mutation UploadProjectFile($teamId: ID!, $file: Upload!) { + uploadProjectFile(input: {teamId: $teamId, file: $file}) { + projectId + __typename + } + }`, } assert.Nil(t, err) r := RequestWithMultipart(e, uID.String(), requestBody, filePath) - projectData := r.Object().Value("data").Object().Value("importProject").Object().Value("projectData") + projectId := r.Object().Value("data").Object().Value("uploadProjectFile").Object().Value("projectId").Raw().(string) + + // Poll until import status is SUCCESS or timeout + deadline := time.Now().Add(30 * time.Second) + var projectData *httpexpect.Value + + for time.Now().Before(deadline) { + // Query project to check import status + projectQuery := GraphQLRequest{ + OperationName: "GetProject", + Query: `query GetProject($projectId: ID!) { + node(id: $projectId, type: PROJECT) { + ... on Project { + importStatus + # Add other fields you need + } + } + }`, + Variables: map[string]any{ + "projectId": projectId, + }, + } + + projectResp := Request(e, uID.String(), projectQuery) + status := projectResp.Path("$.data.node.importStatus").String().Raw() + + if status == "SUCCESS" { + // Fetch the complete project data + // ... + break + } else if status == "FAILED" { + t.Fatalf("Project import failed") + } + + time.Sleep(1 * time.Second) + }Note: The actual implementation will depend on your API design. This is just a suggestion based on the changes described in the PR.
server/internal/app/file_uploader.go (1)
320-350:⚠️ Potential issue
Receivedcounter is incremented even for duplicate chunks
session.Received++is executed before duplicate detection, so a retransmitted chunk inflates the counter and may signal completion prematurely.- session.UpdatedAt = time.Now() - session.Received++ - - for _, chunk := range session.Chunks { + session.UpdatedAt = time.Now() + + for _, chunk := range session.Chunks { if chunk == fmt.Sprintf("%s_part_%d", fileID, chunkNum) { return session, false, nil } } + + session.Received++This keeps
Receivedequal to the distinct number of chunks written.server/internal/adapter/gql/generated.go (1)
6775-6803:⚠️ Potential issueTypo in enum value –
FAILEshould beFAILEDThe enum
ProjectImportStatuscontains the valueFAILE, which looks like a misspelling ofFAILED.
Because this file is generated, the typo originates from the.graphqlschema (or the Go model). Fixing it there and regenerating will prevent:• runtime errors when the backend tries to marshal an unknown enum value
• confusion for API consumers and documentation generatorsProposed schema fix:
-enum ProjectImportStatus { - NONE - PROCESSING - FAILE - SUCCESS -} +enum ProjectImportStatus { + NONE + PROCESSING + FAILED + SUCCESS +}After correcting the schema, re-run
go generate ./...(or the code-gen command you use) to update this file.
🧹 Nitpick comments (5)
server/internal/adapter/gql/resolver_mutation_project.go (1)
196-210: Remove unused functionThe
replaceOldSceneIDfunction is flagged as unused by the linter. It should be removed since the import project functionality has been refactored.-func replaceOldSceneID(data *[]byte, newScene *scene.Scene) (string, error) { - var d map[string]any - if err := json.Unmarshal(*data, &d); err != nil { - return "", err - } - if s, ok := d["scene"].(map[string]any); ok { - if oldSceneID, ok := s["id"].(string); ok { - - // Replace new scene id - *data = bytes.Replace(*data, []byte(oldSceneID), []byte(newScene.ID().String()), -1) - return oldSceneID, nil - } - } - return "", errors.New("scene id not found") -}🧰 Tools
🪛 GitHub Check: ci-server / ci-server-lint
[failure] 196-196:
funcreplaceOldSceneIDis unused (unused)🪛 golangci-lint (1.64.8)
196-196: func
replaceOldSceneIDis unused(unused)
🪛 GitHub Actions: ci
[error] 196-196: golangci-lint: func
replaceOldSceneIDis unused (unused)server/internal/infrastructure/mongo/mongodoc/project.go (1)
70-74: Minor consistency nit – prefer typed alias over plain stringEvery other enum field (
Visualizer,PublishmentStatus, …) is stored as its string value but still typed in the document struct when possible.
StoringImportStatusasstatus.ProjectImportStatusinstead of plainstringwould give compile-time help when populating the document:-importStatus string +importStatus status.ProjectImportStatusNot mandatory, but keeps the struct symmetrical and prevents accidental typos at the call-site.
Also applies to: 123-124
server/internal/adapter/gql/gqlmodel/models_gen.go (1)
1730-1757: Add helper validation to enum instead of hand-rolled switchesThe generated
IsValiduses an exhaustiveswitch.
A simplemap[string]struct{}orstrings.Containson a constant slice avoids future maintenance every time a new value is added.Not urgent (generated file), but worth upstreaming to the generator template.
server/internal/app/file_uploader.go (2)
155-160: Avoid shadowing the importedstatuspackageThe parameter
status status.ProjectImportStatushides the importedstatuspackage insideUpdateImportStatus, forcing callers to qualify constants elsewhere (status.ProjectImportStatusFaile) but not here.
Rename the parameter, e.g.newStatus, for clarity.-func UpdateImportStatus(..., pid id.ProjectID, status status.ProjectImportStatus) { - _, err := usecases.Project.UpdateImportStatus(ctx, pid, status, op) +func UpdateImportStatus(..., pid id.ProjectID, newStatus status.ProjectImportStatus) { + _, err := usecases.Project.UpdateImportStatus(ctx, pid, newStatus, op) }
359-376: Numeric sort helper can panic on unexpected names
getChunkNumberreturns0on parse errors, sopart_foosorts beforepart_1.
Safer toreturn -1or filter invalid entries out; otherwise you may join chunks in the wrong order silently.Not critical if producer guarantees well-formed names, but worth guarding in publicly exposed endpoints.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
server/e2e/gql_project_export_import_test.go(1 hunks)server/gql/project.graphql(2 hunks)server/internal/adapter/gql/generated.go(11 hunks)server/internal/adapter/gql/gqlmodel/convert_project.go(1 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(2 hunks)server/internal/adapter/gql/resolver_mutation_project.go(2 hunks)server/internal/app/file_uploader.go(9 hunks)server/internal/infrastructure/mongo/mongodoc/project.go(3 hunks)server/internal/usecase/interactor/asset.go(7 hunks)server/internal/usecase/interactor/project.go(7 hunks)server/internal/usecase/interfaces/asset.go(1 hunks)server/internal/usecase/interfaces/project.go(4 hunks)server/pkg/project/builder.go(2 hunks)server/pkg/project/project.go(3 hunks)server/pkg/status/Import_status.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
server/e2e/gql_project_export_import_test.go (1)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:48-80
Timestamp: 2024-09-13T01:19:11.267Z
Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.
🧬 Code Graph Analysis (6)
server/internal/infrastructure/mongo/mongodoc/project.go (1)
server/pkg/status/Import_status.go (1)
ProjectImportStatus(3-3)
server/internal/adapter/gql/resolver_mutation_project.go (2)
server/internal/adapter/gql/gqlmodel/models_gen.go (2)
Visualizer(2155-2155)ProjectImportStatusNone(1717-1717)server/pkg/status/Import_status.go (1)
ProjectImportStatusNone(6-6)
server/pkg/status/Import_status.go (1)
server/internal/adapter/gql/gqlmodel/models_gen.go (1)
ProjectImportStatus(1714-1714)
server/internal/adapter/gql/gqlmodel/convert_project.go (2)
server/internal/adapter/gql/gqlmodel/models_gen.go (2)
Visualizer(2155-2155)ProjectImportStatus(1714-1714)server/pkg/status/Import_status.go (1)
ProjectImportStatus(3-3)
server/internal/usecase/interfaces/project.go (6)
server/internal/adapter/gql/gqlmodel/models_gen.go (4)
Visualizer(2155-2155)ProjectImportStatus(1714-1714)Project(673-702)Project(704-704)server/pkg/status/Import_status.go (1)
ProjectImportStatus(3-3)server/internal/usecase/interactor/project.go (1)
Project(37-54)server/pkg/project/project.go (1)
Project(14-43)server/internal/usecase/repo/project.go (1)
Project(19-33)server/internal/app/file_uploader.go (1)
UpdateImportStatus(155-160)
server/internal/adapter/gql/generated.go (4)
server/internal/usecase/interfaces/project.go (1)
Project(74-91)server/pkg/project/project.go (1)
Project(14-43)server/internal/adapter/gql/gqlmodel/models_gen.go (3)
Project(673-702)Project(704-704)ProjectImportStatus(1714-1714)server/pkg/status/Import_status.go (1)
ProjectImportStatus(3-3)
🪛 GitHub Actions: ci
server/internal/adapter/gql/resolver_mutation_project.go
[error] 196-196: golangci-lint: func replaceOldSceneID is unused (unused)
server/e2e/gql_project_export_import_test.go
[error] 45-45: TestProjectExportImport failed: unexpected HTTP status 422 Unprocessable Entity. GraphQL error: Cannot query field "importProject" on type "Mutation". Panic: interface conversion: interface {} is nil, not string.
🔇 Additional comments (29)
server/internal/adapter/gql/gqlmodel/convert_project.go (1)
54-54: LGTM: New import status field added to Project conversionThe new field
ImportStatushas been correctly added to theToProjectconversion function, ensuring that the import status from the domain model is properly mapped to the GraphQL model.server/pkg/project/builder.go (2)
7-9: LGTM: Status package import addedThe import for the status package is correctly added to support the new import status field.
89-92: LGTM: Builder method for import status addedThe new
ImportStatusmethod follows the established builder pattern correctly, allowing for the setting of the import status during project construction.server/pkg/project/project.go (4)
9-10: LGTM: Status package import addedThe import for the status package is correctly added to support the new import status field.
23-23: LGTM: Import status field added to Project structThe new
importStatusfield is correctly added to theProjectstruct.
98-100: LGTM: Getter method for import status addedThe new
ImportStatus()method follows the established pattern for getter methods in this struct.
160-162: LGTM: Setter method for import status addedThe new
UpdateImportStatus()method follows the established pattern for setter methods in this struct.server/internal/usecase/interfaces/asset.go (1)
42-42: LGTM: Operator parameter added for policy enforcementAdding the
*usecase.Operatorparameter to theImportAssetFilesmethod is a good change that allows for policy enforcement during asset import, which aligns with the PR objectives of "adding a policy check during asset import."server/gql/project.graphql (1)
13-13: LGTM: New import status fieldAdding the
importStatusfield to the Project type properly implements the status tracking mechanism described in the PR objectives.server/internal/adapter/gql/resolver_mutation_project.go (1)
31-39: LGTM: Initial import status in CreateProjectSetting the initial import status to
status.ProjectImportStatusNonewhen creating a new project is the correct approach. This aligns with the PR objectives which state that "For projects created normally, the import status is 'NONE' or null."server/internal/usecase/interfaces/project.go (1)
88-90: Double-check the new ImportProjectData signature
ImportProjectData(ctx, teamID string, projectID *string, data *[]byte, op …)places the optional project ID before the payload.
Call-sites have to pass&session.ProjectID, which is easy to mix up withdata.Consider:
ImportProjectData(ctx, teamID, data, &optionalProjectID, op)to match the natural order “context, identifiers, payload, operator”.
server/internal/app/file_uploader.go (1)
170-177: Race condition: project created only when chunk 0 arrivesIf chunks arrive out-of-order (e.g., 1, 2, 0 …), the first call cannot attach a
ProjectIDto the session, resulting in an emptyProjectIDwhen the background import fires.Mitigations:
- Create the processing project the moment the first chunk for a new
fileIDis seen (regardless of its index).- Or postpone spawning the import goroutine until
session.ProjectIDis non-empty.Leaving as-is risks a nil/empty ID and status update failures.
server/internal/usecase/interactor/asset.go (7)
21-24: New import for policy package added.The policy package import is now being used in the refactored asset creation and import process to enforce workspace policy limits.
58-82: Well-structured refactoring of the Create method.The method has been refactored to use a new helper function
uploadAndSavewhich improves code organization and reusability. The policy retrieval and enforcement logic has been properly maintained while making the code more modular.
130-156: ImportAssetFiles now accepts operator parameter and adds workspace policy checks.The method signature has been updated to accept an
operatorparameter and now retrieves the workspace and policy information before processing assets. This change aligns with the PR objective of adding policy checks during asset import.
164-166: Error handling improved for zip file operations.Changed from returning an error to logging and continuing when zip files can't be opened. This makes the import process more resilient by allowing it to continue even if some files have issues.
181-190: Project image update logic extracted to a separate method.The project image update logic has been modularized into the
updateProjectImagemethod, which is good for code organization and maintainability.
203-211: New updateProjectImage method is well-structured.This new helper method cleanly handles the conditional logic for updating a project's image URL when it matches an imported asset.
213-252: New uploadAndSave helper method improves code organization.This well-structured helper method:
- Uploads the file
- Enforces policy limits
- Creates and saves the asset entity
- Properly handles errors at each step
The method also correctly cleans up uploaded files if policy limits are exceeded, preventing orphaned files.
server/internal/usecase/interactor/project.go (8)
28-33: New import for status package added.The status package import supports the new ProjectImportStatus functionality being implemented in this PR.
96-106: Create method updated to support import status.The Create method now passes the ImportStatus field to the createProject function, aligning with the PR objective of implementing an import status tracking mechanism.
222-237: New UpdateImportStatus method correctly implements status updates.This method provides a clean interface for updating a project's import status, with proper error handling and timestamp updates. This directly supports the PR objective of tracking import status during the import process.
676-716: ImportProjectData updated to support project ID and import status.The method now accepts an optional projectId parameter and sets the import status to PROCESSING when calling createProject. This aligns with the PR objective of creating a temporary project with PROCESSING status at the start of data import.
700-710: Import status set to PROCESSING during project creation.The code correctly implements the requirement to set the import status to PROCESSING when importing a project, which aligns with the PR objective of tracking the import process.
748-760: createProjectInput struct updated with new fields.The struct now includes ProjectID and ImportStatus fields to support the new import status tracking functionality.
800-814: Project ID handling correctly implemented.The code now supports using a provided project ID or generating a new one if not provided. This allows for creating a project with a specific ID during the import process, which is essential for the chunked upload workflow.
814-815: Import status properly set during project creation.The code correctly sets the import status when building a new project, which is a key part of implementing the import status tracking mechanism described in the PR objectives.
server/internal/adapter/gql/generated.go (2)
6775-6778: Non-nullableimportStatusmay crash for legacy projects
importStatus: ProjectImportStatus!is declared non-nullable.
If any existing project record has an empty string ornullin MongoDB, the resolver (_Project_importStatus) will hit themust not be nullbranch (lines 26843-26847) and return a GraphQL error for the entire query.Please verify that:
- All existing documents have
importStatuspopulated—e.g. default"NONE".- New project creation paths set the field explicitly.
If either isn’t true, change the field to nullable or run a data migration.
538-541: Complexity map updated correctly – nothing to addThe new
ImportStatusfield is added to the complexity struct in line with other scalar fields. Looks good.
server/pkg/status/Import_status.go
Outdated
| type ProjectImportStatus string | ||
|
|
||
| const ( | ||
| ProjectImportStatusNone ProjectImportStatus = "NONE" | ||
| ProjectImportStatusProcessing ProjectImportStatus = "PROCESSING" | ||
| ProjectImportStatusFaile ProjectImportStatus = "FAILE" | ||
| ProjectImportStatusSuccess ProjectImportStatus = "SUCCESS" | ||
| ) |
There was a problem hiding this comment.
Fix typo in enum value "FAILE"
There's a typo in the enum value ProjectImportStatusFaile - it should be FAILED instead of FAILE. This could cause confusion for clients consuming the API.
- ProjectImportStatusFaile ProjectImportStatus = "FAILE"
+ ProjectImportStatusFailed ProjectImportStatus = "FAILED"Additionally, consider adding documentation comments to explain the purpose of each status value.
type ProjectImportStatus string
const (
+ // ProjectImportStatusNone indicates no import has been attempted
ProjectImportStatusNone ProjectImportStatus = "NONE"
+ // ProjectImportStatusProcessing indicates an import is currently in progress
ProjectImportStatusProcessing ProjectImportStatus = "PROCESSING"
- ProjectImportStatusFaile ProjectImportStatus = "FAILE"
+ // ProjectImportStatusFailed indicates an import attempt that failed
+ ProjectImportStatusFailed ProjectImportStatus = "FAILED"
+ // ProjectImportStatusSuccess indicates a successful import
ProjectImportStatusSuccess ProjectImportStatus = "SUCCESS"
)🤖 Prompt for AI Agents
In server/pkg/status/Import_status.go between lines 3 and 10, correct the typo
in the enum value by renaming ProjectImportStatusFaile to
ProjectImportStatusFailed and changing its string value from "FAILE" to
"FAILED". Also, add documentation comments above each constant to clearly
describe the meaning of each import status value for better code clarity and API
consumer understanding.
| ImportStatus string | ||
| CoreSupport bool |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate ‟ImportStatus” before round-tripping to the domain model
ImportStatus is persisted as a raw string.
If anything other than the four valid enum values (or an empty string) is stored, Model() will still feed that value back into the builder without any validation, silently polluting the domain model with an invalid enum.
Consider normalising the value on load:
@@
- ImportStatus: string(project.ImportStatus()),
+ ImportStatus: string(project.ImportStatus()),and in Model():
- .ImportStatus(status.ProjectImportStatus(d.ImportStatus)).
+ .ImportStatus(func() status.ProjectImportStatus {
+ s := status.ProjectImportStatus(d.ImportStatus)
+ if !s.IsValid() { // add IsValid() to status package if missing
+ s = status.ProjectImportStatusNone
+ }
+ return s
+ }()).This makes corrupted data self-healing while keeping the DB schema unchanged.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/internal/infrastructure/mongo/mongodoc/project.go around lines 24 to
25, the ImportStatus field is stored as a raw string without validation,
allowing invalid enum values to propagate into the domain model. To fix this,
add validation logic when loading ImportStatus from the database to normalize or
default invalid values to a valid enum or empty string. Also, update the Model()
method to validate and normalize ImportStatus before returning the domain model,
ensuring corrupted or unexpected values are corrected and do not pollute the
domain logic.
| type CreateProjectParam struct { | ||
| WorkspaceID accountdomain.WorkspaceID | ||
| Visualizer visualizer.Visualizer | ||
| Name *string | ||
| Description *string | ||
| CoreSupport *bool | ||
| Visibility *string | ||
| WorkspaceID accountdomain.WorkspaceID | ||
| Visualizer visualizer.Visualizer | ||
| Name *string | ||
| Description *string | ||
| CoreSupport *bool | ||
| Visibility *string | ||
| ImportStatus status.ProjectImportStatus | ||
| } |
There was a problem hiding this comment.
Use pointer for ImportStatus to follow existing optional-field convention
All optional inputs (Name, Description, CoreSupport, …) use pointers so the caller can distinguish “unset” from “set to zero value”.
ImportStatus is currently a value – the caller cannot express “leave unchanged” in UpdateProjectParam and cannot omit the field in CreateProjectParam.
-type CreateProjectParam struct {
+type CreateProjectParam struct {
@@
- ImportStatus status.ProjectImportStatus
+ ImportStatus *status.ProjectImportStatus
}
@@
- ImportStatus status.ProjectImportStatus
+ ImportStatus *status.ProjectImportStatusUpdateImportStatus already exists for explicit changes, so keeping this field optional avoids accidental overrides.
Also applies to: 39-40
🤖 Prompt for AI Agents
In server/internal/usecase/interfaces/project.go around lines 22 to 30 and also
lines 39 to 40, change the ImportStatus field in CreateProjectParam and
UpdateProjectParam structs from a value type to a pointer type. This aligns with
the existing convention for optional fields, allowing callers to distinguish
between "unset" and "set to zero value" and preventing accidental overrides by
making ImportStatus optional.
| type ProjectImportStatus string | ||
|
|
||
| const ( | ||
| ProjectImportStatusNone ProjectImportStatus = "NONE" | ||
| ProjectImportStatusProcessing ProjectImportStatus = "PROCESSING" | ||
| ProjectImportStatusFaile ProjectImportStatus = "FAILE" | ||
| ProjectImportStatusSuccess ProjectImportStatus = "SUCCESS" | ||
| ) |
There was a problem hiding this comment.
Typo: enum value FAILE should be FAILED
The GraphQL-exposed enum has a misspelling:
enum ProjectImportStatus {
NONE
PROCESSING
- FAILE
+ FAILED
SUCCESS
}Downstream clients will carry this typo indefinitely once released.
Fix it in the GraphQL schema and regenerate – do not patch the generated file directly.
🤖 Prompt for AI Agents
In server/internal/adapter/gql/gqlmodel/models_gen.go lines 1714 to 1721, the
enum value "FAILE" is a typo and should be corrected to "FAILED". Do not edit
this generated file directly; instead, fix the typo in the original GraphQL
schema definition by changing "FAILE" to "FAILED" in the ProjectImportStatus
enum, then regenerate the gqlmodel code to propagate the fix properly.
| func (ec *executionContext) _Project_importStatus(ctx context.Context, field graphql.CollectedField, obj *gqlmodel.Project) (ret graphql.Marshaler) { | ||
| fc, err := ec.fieldContext_Project_importStatus(ctx, field) | ||
| if err != nil { | ||
| return graphql.Null | ||
| } | ||
| ctx = graphql.WithFieldContext(ctx, fc) | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| ec.Error(ctx, ec.Recover(ctx, r)) | ||
| ret = graphql.Null | ||
| } | ||
| }() | ||
| resTmp, err := ec.ResolverMiddleware(ctx, func(rctx context.Context) (any, error) { | ||
| ctx = rctx // use context from middleware stack in children | ||
| return obj.ImportStatus, nil | ||
| }) | ||
| if err != nil { | ||
| ec.Error(ctx, err) | ||
| return graphql.Null | ||
| } | ||
| if resTmp == nil { | ||
| if !graphql.HasFieldError(ctx, fc) { | ||
| ec.Errorf(ctx, "must not be null") | ||
| } | ||
| return graphql.Null | ||
| } | ||
| res := resTmp.(gqlmodel.ProjectImportStatus) | ||
| fc.Result = res | ||
| return ec.marshalNProjectImportStatus2githubᚗcomᚋreearthᚋreearthᚋserverᚋinternalᚋadapterᚋgqlᚋgqlmodelᚐProjectImportStatus(ctx, field.Selections, res) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Resolver returns raw enum string – ensure zero value is NEVER empty
The resolver simply returns obj.ImportStatus. If the zero value ("") slips through, GraphQL marshalling will fail because the empty string doesn’t match any enum literal.
Guard at the domain layer (e.g. make the type an alias with const Unknown = "NONE" default) or add a fallback here:
- return obj.ImportStatus, nil
+ if obj.ImportStatus == "" {
+ return gqlmodel.ProjectImportStatusNone, nil // or your default
+ }
+ return obj.ImportStatus, nil(Adjust constant name to match generated enum.)
🤖 Prompt for AI Agents
In server/internal/adapter/gql/generated.go around lines 26823 to 26852, the
resolver returns the raw enum string obj.ImportStatus, which can be an empty
string causing GraphQL marshalling to fail. To fix this, add a guard in the
resolver to check if obj.ImportStatus is empty and return a default fallback
enum value (e.g., "NONE" or an appropriate constant) instead. This ensures the
resolver never returns an invalid empty enum string.
734c963 to
3c3b838
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/internal/adapter/gql/generated.go (1)
26823-26852:⚠️ Potential issueCritical issue still exists - resolver returns raw enum string without validation.
The resolver directly returns
obj.ImportStatuswithout checking for empty string values, which can cause GraphQL marshalling failures when the zero value slips through. This is the exact same issue flagged in the previous review.Apply this fix to handle empty import status values:
- return obj.ImportStatus, nil + if obj.ImportStatus == "" { + return gqlmodel.ProjectImportStatusNone, nil + } + return obj.ImportStatus, nil
🧹 Nitpick comments (1)
server/internal/usecase/interactor/project.go (1)
222-237: Consider adding transaction support for data consistencyThe
UpdateImportStatusmethod updates the project's import status and timestamp, but unlike other update methods in this file (e.g.,Update,Publish), it doesn't use a database transaction. Consider whether this operation should be wrapped in a transaction for consistency with other project updates.func (i *Project) UpdateImportStatus(ctx context.Context, pid id.ProjectID, importStatus status.ProjectImportStatus, operator *usecase.Operator) (*project.Project, error) { + tx, err := i.transaction.Begin(ctx) + if err != nil { + return nil, err + } + + ctx = tx.Context() + defer func() { + if err2 := tx.End(ctx); err == nil && err2 != nil { + err = err2 + } + }() + prj, err := i.projectRepo.FindByID(ctx, pid) if err != nil { return nil, err } prj.UpdateImportStatus(importStatus) currentTime := time.Now().UTC() prj.SetUpdatedAt(currentTime) if err := i.projectRepo.Save(ctx, prj); err != nil { return nil, err } + + tx.Commit() return prj, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
server/e2e/gql_project_export_import_test.go(0 hunks)server/e2e/gql_scene_test.go(1 hunks)server/gql/project.graphql(2 hunks)server/internal/adapter/gql/generated.go(11 hunks)server/internal/adapter/gql/gqlmodel/convert_project.go(1 hunks)server/internal/adapter/gql/gqlmodel/convert_project_test.go(1 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(2 hunks)server/internal/adapter/gql/resolver_mutation_project.go(2 hunks)server/internal/app/file_uploader.go(9 hunks)server/internal/infrastructure/mongo/mongodoc/project.go(3 hunks)server/internal/infrastructure/mongo/project_test.go(2 hunks)server/internal/usecase/interactor/asset.go(7 hunks)server/internal/usecase/interactor/project.go(7 hunks)server/internal/usecase/interfaces/asset.go(1 hunks)server/internal/usecase/interfaces/project.go(4 hunks)server/pkg/project/builder.go(2 hunks)server/pkg/project/project.go(3 hunks)server/pkg/status/Import_status.go(1 hunks)
💤 Files with no reviewable changes (1)
- server/e2e/gql_project_export_import_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- server/pkg/project/builder.go
- server/pkg/status/Import_status.go
- server/internal/adapter/gql/gqlmodel/convert_project.go
- server/internal/usecase/interfaces/asset.go
- server/pkg/project/project.go
- server/internal/infrastructure/mongo/mongodoc/project.go
- server/internal/adapter/gql/resolver_mutation_project.go
- server/internal/usecase/interfaces/project.go
- server/e2e/gql_scene_test.go
- server/internal/app/file_uploader.go
- server/internal/adapter/gql/gqlmodel/models_gen.go
- server/gql/project.graphql
- server/internal/usecase/interactor/asset.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/internal/infrastructure/mongo/project_test.go (3)
server/pkg/project/builder.go (1)
New(17-19)server/internal/adapter/gql/gqlmodel/models_gen.go (4)
PublishmentStatus(1869-1869)PublishmentStatusPublic(1872-1872)ProjectImportStatusNone(1717-1717)PublishmentStatusLimited(1873-1873)server/pkg/status/Import_status.go (1)
ProjectImportStatusNone(6-6)
🔇 Additional comments (16)
server/internal/usecase/interactor/project.go (7)
28-28: LGTM - Import addition for status packageThe import of the status package is necessary for the new
ProjectImportStatustype used in the import status functionality.
32-32: LGTM - Import addition for idx packageThe idx package import is required for the enhanced project ID handling in the
createProjectmethod.
98-105: LGTM - Create method updated for import status supportThe Create method correctly forwards the new
ImportStatusfield to thecreateProjectfunction, maintaining consistency with the existing parameter passing pattern.
676-676: LGTM - Method signature enhanced for targeted importsThe addition of the optional
projectIdparameter toImportProjectDataenables importing data into a specific project, which aligns with the asynchronous import workflow described in the PR objectives.
699-710: LGTM - Import data creation with proper statusThe
ImportProjectDatamethod correctly:
- Sets the import status to
status.ProjectImportStatusProcessing- Passes the optional
projectIdtocreateProject- Maintains existing functionality while adding new import tracking capabilities
This implementation aligns with the PR description where imports start with "PROCESSING" status.
749-760: LGTM - Struct expanded for enhanced project creationThe
createProjectInputstruct has been appropriately extended withProjectIDandImportStatusfields to support the new import functionality while maintaining backward compatibility.
800-814: LGTM - Robust project ID handling with proper error managementThe project ID assignment logic is well-implemented:
- Properly parses the provided project ID string with error handling
- Falls back to generating a new ID when none is provided
- Uses the new
ImportStatusfield in project creationThe error handling for ID parsing ensures the system fails gracefully with invalid input.
server/internal/adapter/gql/generated.go (8)
538-538: LGTM - Complexity function added correctly.The
ImportStatuscomplexity function has been properly added to the Project struct.
3758-3763: LGTM - Complexity case handling implemented correctly.The complexity case for "Project.importStatus" follows the standard pattern with proper nil check and complexity calculation.
6775-6775: LGTM - Non-nullable field correctly defined.The
importStatusfield is properly declared as non-nullable (ProjectImportStatus!), which aligns with the requirement that projects should always have an import status.
6798-6803: LGTM - Enum definition is correct.The
ProjectImportStatusenum is properly defined with all expected values: NONE, PROCESSING, FAILED, and SUCCESS.
26854-26865: LGTM - Field context function implemented correctly.The field context function for
importStatusfollows the standard pattern for enum fields.
27809-27810: LGTM - Field context cases added consistently.The "importStatus" cases have been properly added to all relevant field context switch statements.
Also applies to: 28050-28051, 28152-28153, 34522-34523
51704-51708: LGTM - Field execution properly implemented.The execution of
_Project_importStatusis correctly integrated with proper null handling.
58403-58411: LGTM - Marshalling functions implemented correctly.The unmarshalling and marshalling functions for
ProjectImportStatusfollow the standard pattern for GraphQL enums.server/internal/infrastructure/mongo/project_test.go (1)
12-12: LGTM: Import addition is correct.The addition of the
statuspackage import is necessary for usingProjectImportStatusNoneconstant.
| prj1 := project.New().NewID().Workspace(wid).UpdatedAt(now).Alias("alias").PublishmentStatus(project.PublishmentStatusPublic).ImportStatus(status.ProjectImportStatusNone).MustBuild() | ||
| prj2 := project.New().NewID().Workspace(wid).UpdatedAt(now).Alias("aaaaa").PublishmentStatus(project.PublishmentStatusLimited).ImportStatus(status.ProjectImportStatusNone).MustBuild() | ||
| prj3 := project.New().NewID().Workspace(wid).UpdatedAt(now).Alias("bbbbb").MustBuild() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent ImportStatus initialization.
While prj1 and prj2 explicitly set ImportStatus(status.ProjectImportStatusNone), prj3 doesn't have this field set. This inconsistency could lead to different behavior during testing, where prj3 might have a different default ImportStatus value.
Apply this diff to ensure consistency:
prj1 := project.New().NewID().Workspace(wid).UpdatedAt(now).Alias("alias").PublishmentStatus(project.PublishmentStatusPublic).ImportStatus(status.ProjectImportStatusNone).MustBuild()
prj2 := project.New().NewID().Workspace(wid).UpdatedAt(now).Alias("aaaaa").PublishmentStatus(project.PublishmentStatusLimited).ImportStatus(status.ProjectImportStatusNone).MustBuild()
- prj3 := project.New().NewID().Workspace(wid).UpdatedAt(now).Alias("bbbbb").MustBuild()
+ prj3 := project.New().NewID().Workspace(wid).UpdatedAt(now).Alias("bbbbb").ImportStatus(status.ProjectImportStatusNone).MustBuild()🤖 Prompt for AI Agents
In server/internal/infrastructure/mongo/project_test.go around lines 107 to 109,
prj3 is missing the ImportStatus initialization that prj1 and prj2 have, causing
inconsistent test behavior. Add ImportStatus(status.ProjectImportStatusNone) to
prj3's builder chain to ensure all three projects have the same ImportStatus set
explicitly.
| @@ -91,6 +91,7 @@ func TestToProject(t *testing.T) { | |||
| ImageURL: lo.Must(url.Parse("https://example.com/hoge.gif")), | |||
| TeamID: IDFrom(wsId), | |||
| Visualizer: Visualizer(visualizer.VisualizerCesium), | |||
| ImportStatus: "NONE", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use constant instead of hard-coded string for ImportStatus.
The ImportStatus field uses a hard-coded string "NONE" instead of the defined constant. This reduces maintainability and could lead to inconsistencies if the enum value changes.
Apply this diff to use the constant:
- ImportStatus: "NONE",
+ ImportStatus: string(ProjectImportStatusNone),Note: You may need to add the appropriate import or ensure ProjectImportStatusNone is accessible in this test file's scope.
📝 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.
| ImportStatus: "NONE", | |
| ImportStatus: string(ProjectImportStatusNone), |
🤖 Prompt for AI Agents
In server/internal/adapter/gql/gqlmodel/convert_project_test.go at line 94,
replace the hard-coded string "NONE" assigned to ImportStatus with the
corresponding constant ProjectImportStatusNone. Ensure that the constant is
properly imported or accessible in the test file to maintain consistency and
improve maintainability.
hexaforce
force-pushed
the
feat/add-project-import-status
branch
from
3c3b838 to
4430b19
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
server/internal/adapter/gql/generated.go (1)
26823-26852:⚠️ Potential issueAddress the empty string handling issue (duplicate from past review).
This is the same issue identified in the previous review. The resolver returns
obj.ImportStatusdirectly without checking if it's an empty string, which could cause GraphQL marshaling failures when the zero value slips through.Apply the previously suggested fix:
- return obj.ImportStatus, nil + if obj.ImportStatus == "" { + return gqlmodel.ProjectImportStatusNone, nil + } + return obj.ImportStatus, nil
🧹 Nitpick comments (1)
server/internal/app/file_uploader.go (1)
359-375: Improve chunk sorting robustness and performance.The current sorting implementation has some issues:
- Performance: Creating a closure for each comparison is inefficient
- Error handling: Parse errors result in chunk number 0, which could cause incorrect ordering
- Logging: Warning logs in comparison function could be excessive
Here's a more robust implementation:
+ // Pre-compute chunk numbers for efficient sorting + type chunkInfo struct { + name string + number int + } + + chunks := make([]chunkInfo, len(session.Chunks)) + for i, chunk := range session.Chunks { + parts := strings.Split(chunk, "_part_") + if len(parts) != 2 { + log.Printf("warning: unexpected chunk name format: %s", chunk) + chunks[i] = chunkInfo{name: chunk, number: 0} + continue + } + n, err := strconv.Atoi(parts[1]) + if err != nil { + log.Printf("warning: failed to parse chunk number from %s: %v", chunk, err) + chunks[i] = chunkInfo{name: chunk, number: 0} + continue + } + chunks[i] = chunkInfo{name: chunk, number: n} + } - sort.Slice(session.Chunks, func(i, j int) bool { - getChunkNumber := func(s string) int { - parts := strings.Split(s, "_part_") - if len(parts) != 2 { - log.Printf("warning: unexpected chunk name format: %s", s) - return 0 - } - n, err := strconv.Atoi(parts[1]) - if err != nil { - log.Printf("warning: failed to parse chunk number from %s: %v", s, err) - return 0 - } - return n - } - - return getChunkNumber(session.Chunks[i]) < getChunkNumber(session.Chunks[j]) - }) + sort.Slice(chunks, func(i, j int) bool { + return chunks[i].number < chunks[j].number + }) + + // Update session.Chunks with sorted names + for i, chunk := range chunks { + session.Chunks[i] = chunk.name + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
server/e2e/gql_project_export_import_test.go(0 hunks)server/e2e/gql_scene_test.go(1 hunks)server/gql/project.graphql(2 hunks)server/internal/adapter/gql/generated.go(11 hunks)server/internal/adapter/gql/gqlmodel/convert_project.go(1 hunks)server/internal/adapter/gql/gqlmodel/convert_project_test.go(1 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(2 hunks)server/internal/adapter/gql/resolver_mutation_project.go(2 hunks)server/internal/app/file_uploader.go(9 hunks)server/internal/infrastructure/mongo/mongodoc/project.go(3 hunks)server/internal/infrastructure/mongo/project_test.go(2 hunks)server/internal/usecase/interactor/asset.go(7 hunks)server/internal/usecase/interactor/project.go(8 hunks)server/internal/usecase/interfaces/asset.go(1 hunks)server/internal/usecase/interfaces/project.go(4 hunks)server/pkg/project/builder.go(2 hunks)server/pkg/project/project.go(3 hunks)server/pkg/status/Import_status.go(1 hunks)
💤 Files with no reviewable changes (1)
- server/e2e/gql_project_export_import_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- server/internal/infrastructure/mongo/project_test.go
- server/internal/adapter/gql/gqlmodel/convert_project.go
- server/internal/adapter/gql/gqlmodel/convert_project_test.go
- server/pkg/project/builder.go
- server/gql/project.graphql
- server/pkg/project/project.go
- server/pkg/status/Import_status.go
- server/internal/usecase/interfaces/asset.go
- server/internal/infrastructure/mongo/mongodoc/project.go
- server/internal/adapter/gql/resolver_mutation_project.go
- server/e2e/gql_scene_test.go
- server/internal/usecase/interfaces/project.go
- server/internal/usecase/interactor/project.go
- server/internal/adapter/gql/gqlmodel/models_gen.go
- server/internal/usecase/interactor/asset.go
🔇 Additional comments (12)
server/internal/app/file_uploader.go (4)
15-17: LGTM: Import additions support new functionality.The new imports are appropriate for the enhanced project import status tracking functionality.
Also applies to: 28-34
48-48: LGTM: ProjectID field addition aligns with new workflow.Adding ProjectID to the UploadSession struct is necessary to track the project created during the chunked upload process.
320-320: LGTM: Session update logic correctly handles project assignment.The changes to accept an optional project parameter and store its ID are well-implemented.
Also applies to: 333-335
220-220:✅ Verification successful
Verify parameter changes in usecase method calls.
The import methods now receive additional parameters (
&session.ProjectIDandnewProject). Ensure these changes are consistent with the updated method signatures in the usecase layer.Also applies to: 227-227
🏁 Script executed:
#!/bin/bash # Description: Verify that the usecase methods accept the new parameters # Expected: Method signatures should match the new parameter usage # Check ImportProjectData method signature ast-grep --pattern 'func ($$$) ImportProjectData($$$) ($$$) { $$$ }' # Check ImportAssetFiles method signature ast-grep --pattern 'func ($$$) ImportAssetFiles($$$) ($$$) { $$$ }'Length of output: 8669
Import method parameter verification
Verified that both
ImportProjectDataandImportAssetFilesin the usecase layer accept the newly added parameters:
ImportProjectData(ctx context.Context, workspace string, projectId *string, data *[]byte, op *usecase.Operator) (*project.Project, error)ImportAssetFiles(ctx context.Context, assets map[string]*zip.File, data *[]byte, newProject *project.Project, operator *usecase.Operator) (*[]byte, error)No further changes are needed.
server/internal/adapter/gql/generated.go (8)
538-538: LGTM! Complexity function properly added.The
ImportStatuscomplexity function is correctly added to the Project complexity struct, maintaining consistency with other field complexity functions.
3758-3763: LGTM! Complexity calculation properly implemented.The complexity calculation for
Project.importStatusfollows the standard pattern with proper nil checking and complexity function invocation.
6775-6775: LGTM! Non-nullable field correctly defined.The
importStatusfield is properly defined as non-nullable (ProjectImportStatus!) in the GraphQL schema, which aligns with the business requirement that every project should have an import status.
6798-6803: LGTM! Enum values are well-defined.The
ProjectImportStatusenum includes all necessary states for tracking import progress: NONE (default), PROCESSING (in progress), FAILED (error state), and SUCCESS (completed). The values are clear and follow GraphQL naming conventions.
26854-26865: LGTM! Field context properly implemented.The field context function for
importStatusis correctly implemented with proper error handling for child field access attempts (which should return an error since enums don't have child fields).
27809-27810: LGTM! Field context routing properly added.The
importStatusfield context routing is consistently added across all relevant GraphQL execution contexts, maintaining proper field resolution paths.Also applies to: 28050-28051, 28152-28153, 34522-34523
51704-51708: LGTM! Field execution properly implemented.The
importStatusfield execution is correctly added to the Project field resolution with proper null checking and invalid counter incrementing, following the same pattern as other non-nullable fields.
58403-58411: LGTM! Marshaling functions properly implemented.The unmarshal and marshal functions for
ProjectImportStatusare correctly implemented, delegating to the enum's ownUnmarshalGQLmethod and returning the enum value directly for marshaling.
| var prj *project.Project | ||
| if chunkNum == 0 { | ||
| prj, err = CreateProcessingProject(ctx, usecases, op, teamId) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential security vulnerability: Unrestricted project creation.
Creating a project automatically on the first chunk (chunk 0) could enable abuse scenarios:
- DoS attacks: Malicious users could send multiple chunk 0 requests to create numerous projects
- Resource exhaustion: No apparent rate limiting or validation before project creation
Consider adding validation and rate limiting:
var prj *project.Project
if chunkNum == 0 {
+ // Validate team permissions and rate limits before creating project
+ if err := validateProjectCreation(ctx, usecases, op, teamId); err != nil {
+ return nil, echo.NewHTTPError(http.StatusForbidden, "Project creation not allowed")
+ }
prj, err = CreateProcessingProject(ctx, usecases, op, teamId)
if err != nil {
return nil, err
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/internal/app/file_uploader.go around lines 170 to 177, the code
creates a project automatically when chunkNum is 0 without any validation or
rate limiting, which can lead to DoS attacks and resource exhaustion. To fix
this, add validation checks to ensure the request is authorized and legitimate
before creating a project, and implement rate limiting to restrict how
frequently projects can be created by a user or IP. This will prevent abuse by
controlling project creation frequency and validating inputs.
| func CreateProcessingProject(ctx context.Context, usecases *interfaces.Container, op *usecase.Operator, teamId string) (*project.Project, error) { | ||
|
|
||
| visibility := "private" | ||
| coreSupport := true | ||
| unknown := "It's importing now..." | ||
| workspaceID, err := accountdomain.WorkspaceIDFrom(teamId) | ||
| if err != nil { | ||
| return nil, echo.NewHTTPError(http.StatusBadRequest, "Invalid team id") | ||
| } | ||
|
|
||
| prj, err := usecases.Project.Create(ctx, interfaces.CreateProjectParam{ | ||
| WorkspaceID: workspaceID, | ||
| Visualizer: visualizer.VisualizerCesium, | ||
| Name: &unknown, | ||
| Description: &unknown, | ||
| CoreSupport: &coreSupport, | ||
| Visibility: &visibility, | ||
| ImportStatus: status.ProjectImportStatusProcessing, // PROCESSING | ||
| }, op) | ||
| if err != nil { | ||
| return nil, echo.NewHTTPError(http.StatusBadRequest, "Failed to create project") | ||
| } | ||
|
|
||
| return prj, nil | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review hardcoded project configuration and error handling.
The project creation logic has some concerns:
- Hardcoded values: Using "It's importing now..." for both name and description may not be user-friendly
- Error type inconsistency: Returning
echo.NewHTTPErrorfrom a non-HTTP handler function is inappropriate
Consider this refactor:
func CreateProcessingProject(ctx context.Context, usecases *interfaces.Container, op *usecase.Operator, teamId string) (*project.Project, error) {
visibility := "private"
coreSupport := true
- unknown := "It's importing now..."
+ name := "Importing Project"
+ description := "Project being imported via chunked upload"
workspaceID, err := accountdomain.WorkspaceIDFrom(teamId)
if err != nil {
- return nil, echo.NewHTTPError(http.StatusBadRequest, "Invalid team id")
+ return nil, fmt.Errorf("invalid team id: %w", err)
}
prj, err := usecases.Project.Create(ctx, interfaces.CreateProjectParam{
WorkspaceID: workspaceID,
Visualizer: visualizer.VisualizerCesium,
- Name: &unknown,
- Description: &unknown,
+ Name: &name,
+ Description: &description,
CoreSupport: &coreSupport,
Visibility: &visibility,
ImportStatus: status.ProjectImportStatusProcessing,
}, op)
if err != nil {
- return nil, echo.NewHTTPError(http.StatusBadRequest, "Failed to create project")
+ return nil, fmt.Errorf("failed to create project: %w", err)
}
return prj, 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.
| func CreateProcessingProject(ctx context.Context, usecases *interfaces.Container, op *usecase.Operator, teamId string) (*project.Project, error) { | |
| visibility := "private" | |
| coreSupport := true | |
| unknown := "It's importing now..." | |
| workspaceID, err := accountdomain.WorkspaceIDFrom(teamId) | |
| if err != nil { | |
| return nil, echo.NewHTTPError(http.StatusBadRequest, "Invalid team id") | |
| } | |
| prj, err := usecases.Project.Create(ctx, interfaces.CreateProjectParam{ | |
| WorkspaceID: workspaceID, | |
| Visualizer: visualizer.VisualizerCesium, | |
| Name: &unknown, | |
| Description: &unknown, | |
| CoreSupport: &coreSupport, | |
| Visibility: &visibility, | |
| ImportStatus: status.ProjectImportStatusProcessing, // PROCESSING | |
| }, op) | |
| if err != nil { | |
| return nil, echo.NewHTTPError(http.StatusBadRequest, "Failed to create project") | |
| } | |
| return prj, nil | |
| } | |
| func CreateProcessingProject(ctx context.Context, usecases *interfaces.Container, op *usecase.Operator, teamId string) (*project.Project, error) { | |
| visibility := "private" | |
| coreSupport := true | |
| name := "Importing Project" | |
| description := "Project being imported via chunked upload" | |
| workspaceID, err := accountdomain.WorkspaceIDFrom(teamId) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid team id: %w", err) | |
| } | |
| prj, err := usecases.Project.Create(ctx, interfaces.CreateProjectParam{ | |
| WorkspaceID: workspaceID, | |
| Visualizer: visualizer.VisualizerCesium, | |
| Name: &name, | |
| Description: &description, | |
| CoreSupport: &coreSupport, | |
| Visibility: &visibility, | |
| ImportStatus: status.ProjectImportStatusProcessing, | |
| }, op) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create project: %w", err) | |
| } | |
| return prj, nil | |
| } |
🤖 Prompt for AI Agents
In server/internal/app/file_uploader.go lines 128 to 153, replace the hardcoded
project name and description "It's importing now..." with more meaningful or
configurable default values to improve user experience. Also, change the error
returns from echo.NewHTTPError to standard Go errors or custom error types
appropriate for non-HTTP handler functions, ensuring consistent and proper error
handling outside HTTP contexts.
| return | ||
| } | ||
|
|
||
| bgctx := context.Background() | ||
|
|
||
| assembledPath := filepath.Join(m.tempDir, session.FileID) | ||
| defer safeRemove(assembledPath) | ||
|
|
||
| if err := m.AssembleChunks(session, assembledPath); err != nil { | ||
| log.Printf("failed to assemble chunks: %v", err) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | ||
| return | ||
| } | ||
|
|
||
| fs := afero.NewOsFs() | ||
| f, err := fs.Open(assembledPath) | ||
| if err != nil { | ||
| log.Printf("failed to open assembled file: %v", err) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | ||
| return | ||
| } | ||
| defer closeWithError(f, &err) | ||
|
|
||
| importData, assetsZip, pluginsZip, err := file_.UncompressExportZip(adapter.CurrentHost(bgctx), f) | ||
| if err != nil { | ||
| log.Printf("fail UncompressExportZip: %v", err) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | ||
| return | ||
| } | ||
|
|
||
| newProject, err := usecases.Project.ImportProjectData(bgctx, teamId, importData, op) | ||
| newProject, err := usecases.Project.ImportProjectData(bgctx, teamId, &session.ProjectID, importData, op) | ||
| if err != nil { | ||
| log.Printf("fail Import ProjectData: %v", err) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | ||
| return | ||
| } | ||
|
|
||
| importData, err = usecases.Asset.ImportAssetFiles(bgctx, assetsZip, importData, newProject) | ||
| importData, err = usecases.Asset.ImportAssetFiles(bgctx, assetsZip, importData, newProject, op) | ||
| if err != nil { | ||
| log.Printf("fail Import AssetFiles: %v", err) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | ||
| return | ||
| } | ||
|
|
||
| newScene, err := usecases.Scene.Create(bgctx, newProject.ID(), false, op) | ||
| if err != nil { | ||
| log.Printf("fail Create Scene: %v", err) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | ||
| return | ||
| } | ||
|
|
||
| oldSceneID, err := replaceOldSceneID(importData, newScene) | ||
| if err != nil { | ||
| log.Printf("fail Get OldSceneID: %v", err) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | ||
| return | ||
| } | ||
|
|
||
| _, _, err = usecases.Plugin.ImportPlugins(bgctx, pluginsZip, oldSceneID, newScene, importData) | ||
| if err != nil { | ||
| log.Printf("fail ImportPlugins: %v", err) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | ||
| return | ||
| } | ||
|
|
||
| newScene, err = usecases.Scene.ImportScene(bgctx, newScene, importData) | ||
| if err != nil { | ||
| log.Printf("fail sceneJSON ImportScene: %v", err) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | ||
| return | ||
| } | ||
|
|
||
| _, err = usecases.Style.ImportStyles(bgctx, newScene.ID(), importData) | ||
| if err != nil { | ||
| log.Printf("fail sceneJSON ImportStyles: %v", err) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | ||
| return | ||
| } | ||
|
|
||
| _, err = usecases.NLSLayer.ImportNLSLayers(bgctx, newScene.ID(), importData) | ||
| if err != nil { | ||
| log.Printf("fail sceneJSON ImportNLSLayers: %v", err) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | ||
| return | ||
| } | ||
|
|
||
| _, err = usecases.StoryTelling.ImportStory(bgctx, newScene.ID(), importData) | ||
| if err != nil { | ||
| log.Printf("fail sceneJSON ImportStory: %v", err) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | ||
| return | ||
| } | ||
|
|
||
| m.CleanupSession(session.FileID) | ||
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusSuccess) // SUCCESS | ||
| log.Printf("Upload completed: %s (%d chunks)", session.FileID, session.TotalChunks) | ||
|
|
||
| }(session) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix context handling and improve error management in async import.
The async goroutine has several issues:
- Context loss: Using
context.Background()loses request tracing and cancellation - Session data race: Accessing
session.ProjectIDwithout synchronization - Resource cleanup: No cleanup on early failures
Improve the async import handling:
if completed {
// Import process, this process will take some time
go func(session *UploadSession) {
+ // Create a context with timeout for the import process
+ importCtx, cancel := context.WithTimeout(context.Background(), 30*time.Minute)
+ defer cancel()
- pid, err := id.ProjectIDFrom(session.ProjectID)
+ // Copy ProjectID to avoid race condition
+ projectIDStr := session.ProjectID
+ pid, err := id.ProjectIDFrom(projectIDStr)
if err != nil {
log.Printf("Invalid project id: %v", err)
return
}
- bgctx := context.Background()
+ // Ensure cleanup on any failure
+ defer func() {
+ if r := recover(); r != nil {
+ log.Printf("Import process panicked: %v", r)
+ UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed)
+ }
+ m.CleanupSession(session.FileID)
+ }()
assembledPath := filepath.Join(m.tempDir, session.FileID)
defer safeRemove(assembledPath)
if err := m.AssembleChunks(session, assembledPath); err != nil {
log.Printf("failed to assemble chunks: %v", err)
- UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed)
+ UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed)
return
}
// Continue with the rest of the import process...
// (Apply similar context changes throughout)
- m.CleanupSession(session.FileID)
- UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusSuccess)
+ UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusSuccess)
log.Printf("Upload completed: %s (%d chunks)", session.FileID, session.TotalChunks)
}(session)
}📝 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.
| go func(session *UploadSession) { | |
| pid, err := id.ProjectIDFrom(session.ProjectID) | |
| if err != nil { | |
| log.Printf("Invalid project id: %v", err) | |
| return | |
| } | |
| bgctx := context.Background() | |
| assembledPath := filepath.Join(m.tempDir, session.FileID) | |
| defer safeRemove(assembledPath) | |
| if err := m.AssembleChunks(session, assembledPath); err != nil { | |
| log.Printf("failed to assemble chunks: %v", err) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| fs := afero.NewOsFs() | |
| f, err := fs.Open(assembledPath) | |
| if err != nil { | |
| log.Printf("failed to open assembled file: %v", err) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| defer closeWithError(f, &err) | |
| importData, assetsZip, pluginsZip, err := file_.UncompressExportZip(adapter.CurrentHost(bgctx), f) | |
| if err != nil { | |
| log.Printf("fail UncompressExportZip: %v", err) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| newProject, err := usecases.Project.ImportProjectData(bgctx, teamId, importData, op) | |
| newProject, err := usecases.Project.ImportProjectData(bgctx, teamId, &session.ProjectID, importData, op) | |
| if err != nil { | |
| log.Printf("fail Import ProjectData: %v", err) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| importData, err = usecases.Asset.ImportAssetFiles(bgctx, assetsZip, importData, newProject) | |
| importData, err = usecases.Asset.ImportAssetFiles(bgctx, assetsZip, importData, newProject, op) | |
| if err != nil { | |
| log.Printf("fail Import AssetFiles: %v", err) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| newScene, err := usecases.Scene.Create(bgctx, newProject.ID(), false, op) | |
| if err != nil { | |
| log.Printf("fail Create Scene: %v", err) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| oldSceneID, err := replaceOldSceneID(importData, newScene) | |
| if err != nil { | |
| log.Printf("fail Get OldSceneID: %v", err) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| _, _, err = usecases.Plugin.ImportPlugins(bgctx, pluginsZip, oldSceneID, newScene, importData) | |
| if err != nil { | |
| log.Printf("fail ImportPlugins: %v", err) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| newScene, err = usecases.Scene.ImportScene(bgctx, newScene, importData) | |
| if err != nil { | |
| log.Printf("fail sceneJSON ImportScene: %v", err) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| _, err = usecases.Style.ImportStyles(bgctx, newScene.ID(), importData) | |
| if err != nil { | |
| log.Printf("fail sceneJSON ImportStyles: %v", err) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| _, err = usecases.NLSLayer.ImportNLSLayers(bgctx, newScene.ID(), importData) | |
| if err != nil { | |
| log.Printf("fail sceneJSON ImportNLSLayers: %v", err) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| _, err = usecases.StoryTelling.ImportStory(bgctx, newScene.ID(), importData) | |
| if err != nil { | |
| log.Printf("fail sceneJSON ImportStory: %v", err) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| m.CleanupSession(session.FileID) | |
| UpdateImportStatus(bgctx, usecases, op, pid, status.ProjectImportStatusSuccess) // SUCCESS | |
| log.Printf("Upload completed: %s (%d chunks)", session.FileID, session.TotalChunks) | |
| }(session) | |
| go func(session *UploadSession) { | |
| // Create a context with timeout for the import process | |
| importCtx, cancel := context.WithTimeout(context.Background(), 30*time.Minute) | |
| defer cancel() | |
| // Copy ProjectID to avoid race condition | |
| projectIDStr := session.ProjectID | |
| pid, err := id.ProjectIDFrom(projectIDStr) | |
| if err != nil { | |
| log.Printf("Invalid project id: %v", err) | |
| return | |
| } | |
| // Ensure cleanup on any failure (including panics) | |
| defer func() { | |
| if r := recover(); r != nil { | |
| log.Printf("Import process panicked: %v", r) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| } | |
| m.CleanupSession(session.FileID) | |
| }() | |
| assembledPath := filepath.Join(m.tempDir, session.FileID) | |
| defer safeRemove(assembledPath) | |
| if err := m.AssembleChunks(session, assembledPath); err != nil { | |
| log.Printf("failed to assemble chunks: %v", err) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| fs := afero.NewOsFs() | |
| f, err := fs.Open(assembledPath) | |
| if err != nil { | |
| log.Printf("failed to open assembled file: %v", err) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| defer closeWithError(f, &err) | |
| importData, assetsZip, pluginsZip, err := file_.UncompressExportZip(adapter.CurrentHost(importCtx), f) | |
| if err != nil { | |
| log.Printf("fail UncompressExportZip: %v", err) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| newProject, err := usecases.Project.ImportProjectData(importCtx, teamId, &projectIDStr, importData, op) | |
| if err != nil { | |
| log.Printf("fail Import ProjectData: %v", err) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| importData, err = usecases.Asset.ImportAssetFiles(importCtx, assetsZip, importData, newProject, op) | |
| if err != nil { | |
| log.Printf("fail Import AssetFiles: %v", err) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| newScene, err := usecases.Scene.Create(importCtx, newProject.ID(), false, op) | |
| if err != nil { | |
| log.Printf("fail Create Scene: %v", err) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| oldSceneID, err := replaceOldSceneID(importData, newScene) | |
| if err != nil { | |
| log.Printf("fail Get OldSceneID: %v", err) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| _, _, err = usecases.Plugin.ImportPlugins(importCtx, pluginsZip, oldSceneID, newScene, importData) | |
| if err != nil { | |
| log.Printf("fail ImportPlugins: %v", err) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| newScene, err = usecases.Scene.ImportScene(importCtx, newScene, importData) | |
| if err != nil { | |
| log.Printf("fail sceneJSON ImportScene: %v", err) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| _, err = usecases.Style.ImportStyles(importCtx, newScene.ID(), importData) | |
| if err != nil { | |
| log.Printf("fail sceneJSON ImportStyles: %v", err) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| _, err = usecases.NLSLayer.ImportNLSLayers(importCtx, newScene.ID(), importData) | |
| if err != nil { | |
| log.Printf("fail sceneJSON ImportNLSLayers: %v", err) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| _, err = usecases.StoryTelling.ImportStory(importCtx, newScene.ID(), importData) | |
| if err != nil { | |
| log.Printf("fail sceneJSON ImportStory: %v", err) | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusFailed) | |
| return | |
| } | |
| // Final success update | |
| UpdateImportStatus(importCtx, usecases, op, pid, status.ProjectImportStatusSuccess) | |
| log.Printf("Upload completed: %s (%d chunks)", session.FileID, session.TotalChunks) | |
| }(session) |
🤖 Prompt for AI Agents
In server/internal/app/file_uploader.go around lines 185 to 287, the async
goroutine handling the import uses context.Background(), causing loss of request
context and cancellation signals. To fix this, pass the parent context into the
goroutine and use it instead of context.Background(). Also, protect access to
session.ProjectID with proper synchronization or copy its value before the
goroutine to avoid data races. Finally, ensure resource cleanup like removing
temporary files and closing opened files happens on all code paths, including
early returns due to errors, by deferring cleanup functions immediately after
resource acquisition.
Overview
Support for Asynchronous Import Projects
What I've done
1. Fixed Issues with File Uploads
・Fixed a problem with file merging
・Fixed a bug where opening large files would cause an error
・Added a policy check process during asset import
2. Added Import Status During the Import Process
・A temporary project is now created during the first chunk (chunk0)
・At this time, the status returns as "PROCESSING"
・Once the import is complete, the status is set to either SUCCESS or FAILED
・For normally created projects, the status returns as NONE or null
3. Removed ProjectImport from GraphQL
extend type Mutation { createProject(input: CreateProjectInput!): ProjectPayload updateProject(input: UpdateProjectInput!): ProjectPayload publishProject(input: PublishProjectInput!): ProjectPayload deleteProject(input: DeleteProjectInput!): DeleteProjectPayload exportProject(input: ExportProjectInput!): ExportProjectPayload - importProject(input: ImportProjectInput!): ImportProjectPayload }4. Added importStatus to Project
It is necessary to add
importStatuson the web sidetype Project implements Node { ... name: String! description: String! imageUrl: URL visualizer: Visualizer! createdAt: DateTime! updatedAt: DateTime! + importStatus: ProjectImportStatus! ... } +enum ProjectImportStatus { + NONE + PROCESSING + FAILE + SUCCESS +}What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Removals
Improvements