chore(server): move export csv items and GeoJSON ability to Item Usecase Layer #1347
Conversation
WalkthroughThe changes introduce new functionality for exporting items in CSV and GeoJSON formats across multiple components of the server. The implementation adds methods to retrieve and convert items with support for pagination and operator-based access control. New export methods are introduced in the item interactor, interfaces, and adapter layers, enabling flexible data export capabilities with robust error handling and format conversion. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Adapter
participant Interactor
participant Repository
Client->>Adapter: Request Items Export
Adapter->>Interactor: Call ItemsAsGeoJSON/ItemsAsCSV
Interactor->>Repository: Fetch Items with Pagination
Repository-->>Interactor: Return Versioned Items
Interactor->>Interactor: Convert Items to Format
Interactor-->>Adapter: Return Exported Data
Adapter-->>Client: Stream Exported Items
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
✅ Deploy Preview for reearth-cms canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
server/internal/usecase/interactor/item_export.go (2)
33-41: Add error handling for pipe writer close operations.The function ignores errors from
pw.Close()andpw.CloseWithError(). While this might be acceptable in some cases, it's better to log these errors for debugging purposes.Apply this diff to improve error handling:
func handleCSVGeneration(pw *io.PipeWriter, l item.VersionedList, s *schema.Schema) { err := generateCSV(pw, l, s) if err != nil { log.Errorf("failed to generate CSV: %v", err) - _ = pw.CloseWithError(err) + if closeErr := pw.CloseWithError(err); closeErr != nil { + log.Errorf("failed to close pipe writer with error: %v", closeErr) + } } else { - _ = pw.Close() + if closeErr := pw.Close(); closeErr != nil { + log.Errorf("failed to close pipe writer: %v", closeErr) + } } }
42-62: Add timeout mechanism to prevent potential memory leaks.The CSV generation process could potentially run indefinitely without any timeout mechanism, which might lead to memory leaks if clients disconnect unexpectedly.
Consider implementing a timeout mechanism using context:
-func generateCSV(pw *io.PipeWriter, l item.VersionedList, s *schema.Schema) error { +func generateCSV(ctx context.Context, pw *io.PipeWriter, l item.VersionedList, s *schema.Schema) error { w := csv.NewWriter(pw) defer w.Flush() + + done := make(chan error, 1) + go func() { headers := integrationapi.BuildCSVHeaders(s) if err := w.Write(headers); err != nil { - return err + done <- err + return } nonGeoFields := lo.Filter(s.Fields(), func(f *schema.Field, _ int) bool { return !f.IsGeometryField() @@ -53,10 +56,19 @@ row, ok := integrationapi.RowFromItem(ver.Value(), nonGeoFields) if ok { if err := w.Write(row); err != nil { - return err + done <- err + return } } } + done <- w.Error() + }() - return w.Error() + select { + case <-ctx.Done(): + return ctx.Err() + case err := <-done: + return err + } }server/internal/usecase/interactor/item.go (3)
29-30: Ensure type consistency for pagination constantsThe pagination constants have inconsistent types:
maxPerPageis untyped int whiledefaultPerPageis int64. This could lead to unnecessary type conversions.-const maxPerPage = 100 -const defaultPerPage int64 = 50 +const maxPerPage int64 = 100 +const defaultPerPage int64 = 50
1214-1215: Enhance method documentationThe method documentation should include details about the response structure and any validation rules for the GeoJSON conversion.
-// ItemsAsGeoJSON converts items to Geo JSON type given the schema package +// ItemsAsGeoJSON converts items to GeoJSON format based on the schema package. +// It returns a FeatureCollection containing all valid geometry fields from the items. +// Only items with valid geometry fields will be included in the response. +// Returns an error if no geometry fields are found in the schema or if the operator is invalid. func (i Item) ItemsAsGeoJSON(ctx context.Context, schemaPackage *schema.Package, page *int, perPage *int, operator *usecase.Operator) (interfaces.ExportItemsToGeoJSONResponse, error) {
1242-1263: Add documentation and input validationThe helper function lacks documentation and could benefit from additional input validation.
+// fromPagination creates a pagination object from page and perPage parameters. +// If page is nil or <= 0, defaults to 1. +// If perPage is nil, uses defaultPerPage. +// If perPage > maxPerPage, uses maxPerPage. +// Returns a configured pagination object with calculated offset and limit. func fromPagination(page, perPage *int) *usecasex.Pagination { + if page == nil && perPage == nil { + return usecasex.OffsetPagination{ + Offset: 0, + Limit: defaultPerPage, + }.Wrap() + } + p := int64(1) if page != nil && *page > 0 { p = int64(*page) }server/internal/usecase/interactor/item_test.go (2)
1089-1253: Improve test organization and assertions in TestItem_ItemsAsCSVThe test cases are comprehensive but could be improved:
- Consider using test helper functions to reduce duplication in test setup
- Add test cases for concurrent access to the pipe reader
- Add assertions for error cases in pipe reading
Example test helper:
func setupTestSchema(t *testing.T, fields schema.FieldList) (*schema.Schema, *model.Model) { t.Helper() prj := project.New().NewID().Workspace(accountdomain.NewWorkspaceID()).RequestRoles([]workspace.Role{workspace.RoleReader, workspace.RoleWriter}).MustBuild() s := schema.New().NewID().Workspace(prj.Workspace()).Project(prj.ID()).Fields(fields).MustBuild() m := model.New().NewID().Schema(s.ID()).Key(id.RandomKey()).Project(s.Project()).MustBuild() return s, m }
1255-1422: Enhance test coverage in TestItem_ItemsAsGeoJSONThe test cases could be improved:
- Add test cases for invalid GeoJSON data
- Test pagination edge cases (empty pages, last page)
- Add test cases for concurrent access
Example test cases to add:
{ name: "invalid_geojson", args: args{ ctx: context.Background(), schemaPackage: sp1, page: &page1, perPage: &perPage1, op: op, }, seedsItems: item.List{itemWithInvalidGeoJSON}, want: nil, wantError: errInvalidGeoJSON, }, { name: "empty_page", args: args{ ctx: context.Background(), schemaPackage: sp1, page: lo.ToPtr(999), perPage: &perPage1, op: op, }, seedsItems: item.List{i1}, want: emptyFeatureCollection, wantError: nil, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/internal/adapter/integration/item.go(3 hunks)server/internal/usecase/interactor/item.go(3 hunks)server/internal/usecase/interactor/item_export.go(1 hunks)server/internal/usecase/interactor/item_export_test.go(1 hunks)server/internal/usecase/interactor/item_test.go(4 hunks)server/internal/usecase/interfaces/item.go(3 hunks)
🧰 Additional context used
📓 Learnings (1)
server/internal/usecase/interactor/item_export_test.go (1)
Learnt from: jasonkarel
PR: reearth/reearth-cms#1329
File: server/internal/usecase/interactor/item_test.go:1147-1151
Timestamp: 2024-12-10T05:29:09.961Z
Learning: In the test `TestItem_ItemsAsCSV` in `server/internal/usecase/interactor/item_test.go`, negative test cases are intentionally fulfilled by not setting operator permissions, which is acceptable for testing purposes.
🔇 Additional comments (4)
server/internal/usecase/interactor/item_export.go (1)
20-23: LGTM!The function provides a clean abstraction over the
integrationapipackage's functionality.server/internal/usecase/interfaces/item.go (1)
85-92: LGTM!The new types and interface methods are well-defined and properly documented. The design provides a clean separation of concerns for the export functionality.
Also applies to: 113-116
server/internal/usecase/interactor/item_export_test.go (1)
18-97: LGTM! Comprehensive test coverage.The test cases thoroughly cover both success and error scenarios for CSV generation, including:
- Successful export with geometry fields
- Error handling for unsupported point fields
- Various geometry field configurations
server/internal/usecase/interactor/item.go (1)
1202-1206:⚠️ Potential issueFix potential resource leak in pipe handling
The pipe writer is not being closed after writing, which could lead to resource leaks. Additionally, error handling for
csvFromItemscould be improved.pr, pw := io.Pipe() -err = csvFromItems(pw, items, schemaPackage.Schema()) -if err != nil { - return interfaces.ExportItemsToCSVResponse{}, err -} +go func() { + defer pw.Close() + if err := csvFromItems(pw, items, schemaPackage.Schema()); err != nil { + pw.CloseWithError(err) + } +}()⛔ Skipped due to learnings
Learnt from: jasonkarel PR: reearth/reearth-cms#1329 File: server/internal/usecase/interactor/item.go:1235-1239 Timestamp: 2024-12-03T05:29:34.471Z Learning: In the Go code file `server/internal/usecase/interactor/item.go`, the `csvFromItems` function already handles closing the pipe writer, so it's unnecessary to wrap it in a goroutine and close it again in the `ItemsAsCSV` function.
Overview
Moving ItemsAsCSV and ItemsAsGeoJSON data export ability to usecase layer
What I've done
What I haven't done
How I tested
local:
testing in local environment. ensuring csv is generated through /api/models/01jd19w4esd28p6m45sn35mycs/items.csv?page=1&perPage=50&ref=latest
cases:
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates