Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(server): importing items when it contains id #1284

Merged
merged 3 commits into from
Oct 25, 2024
Merged

fix(server): importing items when it contains id #1284

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f86e289
fix importing items when it contains id
yk-eukarya Oct 25, 2024
ae10eb3
fix
yk-eukarya Oct 25, 2024
5deab2a
fix
yk-eukarya Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/e2e/gql_field_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ func createFieldOfEachType(t *testing.T, e *httpexpect.Expect, mId string) fIds
false, false, false, false, "Date",
map[string]any{
"date": map[string]any{
"defaultValue": "2024-01-01T18:06:09+09:00",
"defaultValue": nil,
},
})
tagFId, _ := createField(e, mId, "tag", "tag", "m_tag",
Expand Down
15 changes: 15 additions & 0 deletions server/e2e/integration_asset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,27 @@ package e2e

import (
"net/http"
"strings"
"testing"

"github.com/gavv/httpexpect/v2"
"github.com/reearth/reearth-cms/server/internal/app"
"github.com/reearth/reearth-cms/server/pkg/id"
)

func UploadAsset(e *httpexpect.Expect, pId string, path string, content string) *httpexpect.Value {
res := e.POST("/api/projects/{projectId}/assets", pId).
WithHeader("X-Reearth-Debug-User", uId1.String()).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider making the debug user configurable.

The debug user ID is hardcoded, which could make the function less flexible for different test scenarios.

Consider adding it as a parameter:

-func UploadAsset(e *httpexpect.Expect, pId string, path string, content string) *httpexpect.Value {
+func UploadAsset(e *httpexpect.Expect, pId string, path string, content string, debugUser string) *httpexpect.Value {
     res := e.POST("/api/projects/{projectId}/assets", pId).
-        WithHeader("X-Reearth-Debug-User", uId1.String()).
+        WithHeader("X-Reearth-Debug-User", debugUser).

Committable suggestion was skipped due to low confidence.

WithMultipart().
WithFile("file", path, strings.NewReader(content)).
WithForm(map[string]any{"skipDecompression": true}).
Expect().
Status(http.StatusOK).
JSON()

return res
}

// GET /assets/{assetId}
func TestIntegrationGetAssetAPI(t *testing.T) {
e := StartServer(t, &app.Config{}, true, baseSeeder)
Expand Down
312 changes: 312 additions & 0 deletions server/e2e/integration_item_import_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,312 @@
package e2e

import (
"fmt"
"net/http"
"strings"
"testing"

"github.com/gavv/httpexpect/v2"
"github.com/reearth/reearth-cms/server/internal/app"
)

func IntegrationModelImportMultiPart(e *httpexpect.Expect, mId string, format string, strategy string, mutateSchema bool, geometryFieldKey string, content string) *httpexpect.Value {
res := e.PUT("/api/models/{modelId}/import", mId).
WithHeader("Origin", "https://example.com").
WithHeader("X-Reearth-Debug-User", uId1.String()).
//WithHeader("Content-Type", "multipart/form-data").
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove commented-out code for setting Content-Type header

The line WithHeader("Content-Type", "multipart/form-data") is commented out. Since the WithMultipart() method automatically sets the appropriate Content-Type header, you can remove this commented line to clean up the code.

WithMultipart().
WithFile("file", "./test.geojson", strings.NewReader(content)).
WithFormField("format", format).
WithFormField("strategy", strategy).
WithFormField("mutateSchema", fmt.Sprintf("%t", mutateSchema)).
WithFormField("geometryFieldKey", geometryFieldKey).
Expect().
Status(http.StatusOK).
JSON()

return res
}

func IntegrationModelImportJSON(e *httpexpect.Expect, mId string, assetId string, format string, strategy string, mutateSchema bool, geometryFieldKey *string) *httpexpect.Value {
res := e.PUT("/api/models/{modelId}/import", mId).
WithHeader("Origin", "https://example.com").
WithHeader("X-Reearth-Debug-User", uId1.String()).
WithHeader("Content-Type", "application/json").
WithJSON(map[string]any{
"assetId": assetId,
"format": format,
"strategy": strategy,
"mutateSchema": mutateSchema,
"geometryFieldKey": geometryFieldKey,
}).
Expect().
Status(http.StatusOK).
JSON()

return res
}

// POST /models/{modelId}/import //body: multipart
func TestIntegrationModelImportMultiPart(t *testing.T) {
e := StartServer(t, &app.Config{}, true, baseSeederUser)

pId, _ := createProject(e, wId.String(), "test", "test", "test-1")
mId, _ := createModel(e, pId, "test", "test", "test-1")
fids := createFieldOfEachType(t, e, mId)

// strategy="insert" and mutateSchema=false
fileContent1 := `{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {"type": "Point", "coordinates": [139.28179282584915,36.58570985749664]}, "properties": {"text": "test2"}}]}`
res1 := IntegrationModelImportMultiPart(e, mId, "geoJson", "insert", false, fids.geometryObjectFid, fileContent1)
res1.Object().Value("modelId").String().IsEqual(mId)
res1.Object().Value("itemsCount").Number().IsEqual(1)
res1.Object().Value("insertedCount").Number().IsEqual(1)
res1.Object().Value("updatedCount").Number().IsEqual(0)
res1.Object().Value("ignoredCount").Number().IsEqual(0)
//newFields1 := res1.Object().Value("newFields").Array()
//newFields1.Length().IsEqual(1)
//field1 := newFields1.Value(0).Object()
//field1.Value("key").String().IsEqual(fids.geometryObjectFid)
items1 := IntegrationSearchItem(e, mId, 1, 10, "", "", "", nil)
items1.Object().Value("items").Array().Length().IsEqual(1)

// // strategy="insert" and mutateSchema=true
// fileContent2 := `{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {"type": "Point", "coordinates": [139.28179282584915,36.58570985749664]}, "properties": {"text": "test2"}}]}`
// res2 := IntegrationModelImport(e, mId, "geoJson", "insert", true, fids.geometryObjectFid, fileContent2)

// // strategy="update" and mutateSchema=false
// fileContent3 := `{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {"type": "Point", "coordinates": [139.28179282584915,36.58570985749664]}, "properties": {"text": "test2"}}]}`
// res3 := IntegrationModelImport(e, mId, "geoJson", "update", false, fids.geometryObjectFid, fileContent3)

// // strategy="update" and mutateSchema=true
// fileContent4 := `{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {"type": "Point", "coordinates": [139.28179282584915,36.58570985749664]}, "properties": {"text": "test2"}}]}`
// res4 := IntegrationModelImport(e, mId, "geoJson", "update", true, fids.geometryObjectFid, fileContent4)

// // strategy="upsert" and mutateSchema=false
// fileContent5 := `{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {"type": "Point", "coordinates": [139.28179282584915,36.58570985749664]}, "properties": {"text": "test2"}}]}`
// res5 := IntegrationModelImport(e, mId, "geoJson", "upsert", false, fids.geometryObjectFid, fileContent5)

// // strategy="upsert" and mutateSchema=true
// fileContent6 := `{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {"type": "Point", "coordinates": [139.28179282584915,36.58570985749664]}, "properties": {"text": "test2"}}]}`
// res6 := IntegrationModelImport(e, mId, "geoJson", "upsert", true, fids.geometryObjectFid, fileContent6)
}
Comment on lines +73 to +92
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Clean up commented-out test cases

There are several test cases commented out in this section. If these tests are no longer needed, consider removing them to improve code readability. If they are intended for future use, you might want to track them separately or add a TODO comment explaining when they should be reintroduced.


// POST /models/{modelId}/import //body: json, content: geoJson
func TestIntegrationModelImportJSONWithGeoJsonInput(t *testing.T) {
e := StartServer(t, &app.Config{}, true, baseSeederUser)

pId, _ := createProject(e, wId.String(), "test", "test", "test-1")
mId, _ := createModel(e, pId, "test", "test", "test-1")
fids := createFieldOfEachType(t, e, mId)

// strategy="insert" and mutateSchema=false
fileContent1 := `{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {"type": "Point", "coordinates": [139.28179282584915,36.58570985749664]}, "properties": {"text": "test2"}}]}`
aId := UploadAsset(e, pId, "./test1.geojson", fileContent1).Object().Value("id").String().Raw()
res := IntegrationModelImportJSON(e, mId, aId, "geoJson", "insert", false, &fids.geometryObjectFid)
res.Object().Value("modelId").String().IsEqual(mId)
res.Object().IsEqual(map[string]any{
"modelId": mId,
"itemsCount": 1,
"insertedCount": 1,
"updatedCount": 0,
"ignoredCount": 0,
"newFields": []any{},
})

obj := e.GET("/api/models/{modelId}/items", mId).
// WithHeader("authorization", "Bearer "+secret).
WithHeader("X-Reearth-Debug-User", uId1.String()).
WithQuery("page", 1).
WithQuery("perPage", 5).
Expect().
Status(http.StatusOK).
JSON().
Object().
HasValue("page", 1).
HasValue("perPage", 5).
HasValue("totalCount", 1)

a := obj.Value("items").Array()
a.Length().IsEqual(1)
i := a.Value(0).Object()
i.Value("id").NotNull()
i.Value("fields").Array().Length().IsEqual(2)

// // strategy="insert" and mutateSchema=true
// fileContent2 := `{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {"type": "Point", "coordinates": [239.28179282584915,36.58570985749664]}, "properties": {"text": "test2"}}]}`
// id2 := UploadAsset(e, pId, "./test2.geojson", fileContent2).Object().Value("id").String().Raw()
// res2 := IntegrationModelImportJSON(e, mId, id2, "geoJson", "insert", true, fids.geometryObjectFid)

// // strategy="update" and mutateSchema=false
// fileContent3 := `{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {"type": "Point", "coordinates": [139.28179282584915,36.58570985749664]}, "properties": {"text": "test2"}}]}`
// id3 := UploadAsset(e, pId, "./test3.geojson", fileContent3).Object().Value("id").String().Raw()
// res3 := IntegrationModelImportJSON(e, mId, id3, "geoJson", "update", false, fids.geometryObjectFid)

// // strategy="update" and mutateSchema=true
// fileContent4 := `{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {"type": "Point", "coordinates": [139.28179282584915,36.58570985749664]}, "properties": {"text": "test2"}}]}`
// id4 := UploadAsset(e, pId, "./test4.geojson", fileContent4).Object().Value("id").String().Raw()
// res4 := IntegrationModelImportJSON(e, mId, id4, "geoJson", "update", true, fids.geometryObjectFid)

// // strategy="upsert" and mutateSchema=false
// fileContent5 := `{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {"type": "Point", "coordinates": [139.28179282584915,36.58570985749664]}, "properties": {"text": "test2"}}]}`
// id5 := UploadAsset(e, pId, "./test5.geojson", fileContent5).Object().Value("id").String().Raw()
// res5 := IntegrationModelImportJSON(e, mId, id5, "geoJson", "upsert", false, fids.geometryObjectFid)

// // strategy="upsert" and mutateSchema=true
// fileContent6 := `{"type": "FeatureCollection", "features": [{"type": "Feature", "geometry": {"type": "Point", "coordinates": [139.28179282584915,36.58570985749664]}, "properties": {"text": "test2"}}]}`
// id6 := UploadAsset(e, pId, "./test6.geojson", fileContent6).Object().Value("id").String().Raw()
// res6 := IntegrationModelImportJSON(e, mId, id6, "geoJson", "upsert", true, fids.geometryObjectFid)
}
Comment on lines +135 to +159
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Clean up commented-out test cases

Similar to the previous comment, this block contains commented-out tests. Removing unused code can make the test suite clearer and more maintainable.


// POST /models/{modelId}/import //body: json, content: json, strategy="insert"
func TestIntegrationModelImportJSONWithJsonInput1(t *testing.T) {
e := StartServer(t, &app.Config{}, true, baseSeederUser)

// region strategy="insert" and mutateSchema=false
pId, _ := createProject(e, wId.String(), "test", "test", "test-1")
mId, _ := createModel(e, pId, "test", "test", "test-1")
createFieldOfEachType(t, e, mId)

jsonContent := `[{"text": "test1", "bool": true, "number": 1.1},{"text": "test2", "bool": false, "number": 2},{"text": "test3", "bool": null, "number": null}]`
aId := UploadAsset(e, pId, "./test1.json", jsonContent).Object().Value("id").String().Raw()
res := IntegrationModelImportJSON(e, mId, aId, "json", "insert", false, nil)
res.Object().IsEqual(map[string]any{
"modelId": mId,
"itemsCount": 3,
"insertedCount": 3,
"updatedCount": 0,
"ignoredCount": 0,
"newFields": []any{},
})

obj := e.GET("/api/models/{modelId}/items", mId).
// WithHeader("authorization", "Bearer "+secret).
WithHeader("X-Reearth-Debug-User", uId1.String()).
WithQuery("page", 1).
WithQuery("perPage", 5).
Expect().
Status(http.StatusOK).
JSON().
Object().
HasValue("page", 1).
HasValue("perPage", 5).
HasValue("totalCount", 3)

a := obj.Value("items").Array()
a.Length().IsEqual(3)
i := a.Value(0).Object()
i.Value("id").NotNull()
i.Value("fields").Array().Length().IsEqual(3)
i = a.Value(1).Object()
i.Value("id").NotNull()
i.Value("fields").Array().Length().IsEqual(3)
i = a.Value(2).Object()
i.Value("id").NotNull()
i.Value("fields").Array().Length().IsEqual(1)
// endregion

// region strategy="insert" and mutateSchema=true
mId, _ = createModel(e, pId, "test-2", "test-2", "test-2")

// jsonContent = `[{"text": "test1", "bool": true, "integer": 1},{"text": "test2", "bool": false, "integer": 2}]`
// aId = UploadAsset(e, pId, "./test1.json", jsonContent).Object().Value("id").String().Raw()
res = IntegrationModelImportJSON(e, mId, aId, "json", "insert", true, nil)
res.Object().ContainsSubset(map[string]any{
"modelId": mId,
"itemsCount": 3,
"insertedCount": 3,
"updatedCount": 0,
"ignoredCount": 0,
})
res.Object().Value("newFields").Array().Length().IsEqual(3)

obj = e.GET("/api/models/{modelId}/items", mId).
// WithHeader("authorization", "Bearer "+secret).
WithHeader("X-Reearth-Debug-User", uId1.String()).
WithQuery("page", 1).
WithQuery("perPage", 5).
Expect().
Status(http.StatusOK).
JSON().
Object().
HasValue("page", 1).
HasValue("perPage", 5).
HasValue("totalCount", 3)

a = obj.Value("items").Array()
a.Length().IsEqual(3)
i = a.Value(0).Object()
i.Value("id").NotNull()
i.Value("fields").Array().Length().IsEqual(3)
i = a.Value(1).Object()
i.Value("id").NotNull()
i.Value("fields").Array().Length().IsEqual(3)
i = a.Value(2).Object()
i.Value("id").NotNull()
i.Value("fields").Array().Length().IsEqual(1)
// endregion
}

// POST /models/{modelId}/import //body: json, content: json, strategy="upsert"
func TestIntegrationModelImportJSONWithJsonInput2(t *testing.T) {
e := StartServer(t, &app.Config{}, true, baseSeederUser)

// region strategy="upsert" and mutateSchema=true
pId, _ := createProject(e, wId.String(), "test", "test", "test-1")
mId, _ := createModel(e, pId, "test", "test", "test-1")
f := createFieldOfEachType(t, e, mId)

r := e.POST("/api/models/{modelId}/items", mId).
WithHeader("X-Reearth-Debug-User", uId1.String()).
WithJSON(map[string]interface{}{
"fields": []interface{}{
map[string]string{
"id": f.textFId,
"value": "test value",
},
},
}).
Expect().
Status(http.StatusOK).
JSON().
Object()
iId := r.Value("id").String().Raw()

jsonContent := `[{"id": "` + iId + `","text": "test1", "bool": true, "number": 1.1},{"text": "test2", "bool": false, "number": 2},{"text": "test3", "bool": null, "number": null}]`
aId := UploadAsset(e, pId, "./test1.json", jsonContent).Object().Value("id").String().Raw()
Comment on lines +275 to +276
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Safely construct JSON content to prevent potential injection issues

The jsonContent string is constructed using string concatenation with iId. If iId contains unexpected characters, it could lead to JSON formatting issues or injection vulnerabilities. It's safer to use JSON marshalling to construct the content.

Apply this diff to construct the JSON content safely:

-jsonContent := `[{"id": "` + iId + `","text": "test1", "bool": true, "number": 1.1},{"text": "test2", "bool": false, "number": 2},{"text": "test3", "bool": null, "number": null}]`
+items := []map[string]interface{}{
+    {"id": iId, "text": "test1", "bool": true, "number": 1.1},
+    {"text": "test2", "bool": false, "number": 2},
+    {"text": "test3", "bool": nil, "number": nil},
+}
+jsonBytes, err := json.Marshal(items)
+if err != nil {
+    t.Fatalf("Failed to marshal JSON: %v", err)
+}
+jsonContent := string(jsonBytes)

Ensure you have imported the encoding/json package:

 import (
     "fmt"
     "net/http"
     "strings"
     "testing"
+    "encoding/json"

     "github.com/gavv/httpexpect/v2"
     "github.com/reearth/reearth-cms/server/internal/app"
 )
📝 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.

Suggested change
jsonContent := `[{"id": "` + iId + `","text": "test1", "bool": true, "number": 1.1},{"text": "test2", "bool": false, "number": 2},{"text": "test3", "bool": null, "number": null}]`
aId := UploadAsset(e, pId, "./test1.json", jsonContent).Object().Value("id").String().Raw()
items := []map[string]interface{}{
{"id": iId, "text": "test1", "bool": true, "number": 1.1},
{"text": "test2", "bool": false, "number": 2},
{"text": "test3", "bool": nil, "number": nil},
}
jsonBytes, err := json.Marshal(items)
if err != nil {
t.Fatalf("Failed to marshal JSON: %v", err)
}
jsonContent := string(jsonBytes)
aId := UploadAsset(e, pId, "./test1.json", jsonContent).Object().Value("id").String().Raw()

res := IntegrationModelImportJSON(e, mId, aId, "json", "upsert", true, nil)
res.Object().IsEqual(map[string]any{
"modelId": mId,
"itemsCount": 3,
"insertedCount": 2,
"updatedCount": 1,
"ignoredCount": 0,
"newFields": []any{},
})

obj := e.GET("/api/models/{modelId}/items", mId).
// WithHeader("authorization", "Bearer "+secret).
WithHeader("X-Reearth-Debug-User", uId1.String()).
WithQuery("page", 1).
WithQuery("perPage", 5).
Expect().
Status(http.StatusOK).
JSON().
Object().
HasValue("page", 1).
HasValue("perPage", 5).
HasValue("totalCount", 3)

a := obj.Value("items").Array()
a.Length().IsEqual(3)
i := a.Value(0).Object()
i.Value("id").NotNull().IsEqual(iId)
i.Value("fields").Array().Length().IsEqual(3)
i = a.Value(1).Object()
i.Value("id").NotNull()
i.Value("fields").Array().Length().IsEqual(3)
i = a.Value(2).Object()
i.Value("id").NotNull()
i.Value("fields").Array().Length().IsEqual(1)
// endregion
}
Loading
Loading