feat(server): add JSON export for schema #1299
Conversation
WalkthroughThe changes introduce new functionality for exporting schemas in JSON format, along with comprehensive integration tests for the schema export API endpoints. New methods and endpoints are added to facilitate schema retrieval based on various identifiers, including models and projects. The updates also enhance error handling for unauthorized access and non-existent schemas, ensuring robust API behavior. Additionally, the OpenAPI specification is updated to reflect these new endpoints, maintaining backward compatibility while expanding the API's capabilities. Changes
Possibly related PRs
Suggested reviewers
Poem
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 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…arth/reearth-cms into feat-server/schema-json-export
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
server/internal/adapter/publicapi/schema_export_test.go (1)
35-41: Consider more descriptive subtest names.While using the input type as the test name is good, consider making it more descriptive for better test output.
-t.Run(string(tt.input), func(t *testing.T) { +t.Run(fmt.Sprintf("should map %s to type=%s format=%s", tt.input, tt.wantType, tt.wantFmt), func(t *testing.T) {This would make test failures more immediately understandable without diving into the test code.
server/internal/adapter/integration/schema_export_test.go (1)
11-33: Good use of table-driven tests, consider some enhancements.The test cases provide good coverage of different value types. However, there are a few potential improvements:
- Consider adding descriptive names to test cases to improve readability and error reporting
- Add edge cases for empty or nil values
- Use a typed constant for the "unknown" case instead of a string literal
Here's a suggested enhancement:
tests := []struct { + name string input value.Type wantType string wantFmt string }{ - {value.TypeText, "string", ""}, + { + name: "simple text type", + input: value.TypeText, + wantType: "string", + wantFmt: "", + }, // ... other cases ... - {"unknown", "string", ""}, + { + name: "unknown type should default to string", + input: value.Type("unknown"), + wantType: "string", + wantFmt: "", + }, + { + name: "empty type handling", + input: value.Type(""), + wantType: "string", + wantFmt: "", + }, }server/internal/adapter/integration/schema_export.go (2)
198-204: Consider removing unnecessary project validationThe project validation here seems unnecessary since it's not used in the subsequent operations. The schema lookup by ID is independent of the project context.
Consider removing the project validation block to simplify the code:
- _, err := uc.Project.FindByIDOrAlias(ctx, request.ProjectIdOrAlias, op) - if err != nil { - if errors.Is(err, rerror.ErrNotFound) { - return SchemaByIDWithProjectAsJSON404Response{}, err - } - return SchemaByIDWithProjectAsJSON400Response{}, err - }
287-310: Consider enhancing type mapping with additional JSON Schema featuresThe current type mapping could be enhanced with additional JSON Schema features for better validation:
- For URLs, consider adding
patternfor URL validation- For assets, consider adding
contentEncodingandcontentMediaType- For geometry types, consider adding specific GeoJSON schema references
Example enhancement for URL type:
case value.TypeURL: - return "string", "uri" + return "string", "uri-reference" + // Additional validation could be added in buildProperties: + // fieldSchema["pattern"] = "^https?://.+"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
server/internal/adapter/integration/schema_export.go(1 hunks)server/internal/adapter/integration/schema_export_test.go(1 hunks)server/internal/adapter/publicapi/schema_export.go(1 hunks)server/internal/adapter/publicapi/schema_export_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/adapter/publicapi/schema_export.go
🔇 Additional comments (5)
server/internal/adapter/publicapi/schema_export_test.go (2)
1-8: LGTM! Clean and appropriate imports.
The package declaration and imports are minimal and appropriate for a test file.
11-33: Add comments to group related test cases.
While the test cases are comprehensive, adding comments to group related test cases would improve readability.
Consider this structure:
tests := []struct {
input value.Type
wantType string
wantFmt string
}{
+ // String-based types
{value.TypeText, "string", ""},
{value.TypeTextArea, "string", ""},
{value.TypeRichText, "string", ""},
{value.TypeMarkdown, "string", ""},
{value.TypeSelect, "string", ""},
{value.TypeTag, "string", ""},
{value.TypeReference, "string", ""},
+ // Numeric types
{value.TypeInteger, "integer", ""},
{value.TypeNumber, "number", ""},
+ // Other primitive types
{value.TypeBool, "boolean", ""},
+ // Types with special formats
{value.TypeDateTime, "string", "date-time"},
{value.TypeURL, "string", "uri"},
{value.TypeAsset, "string", "binary"},
+ // Complex types
{value.TypeGroup, "array", ""},
{value.TypeGeometryObject, "object", ""},
{value.TypeGeometryEditor, "object", ""},
+ // Edge cases
{"unknown", "string", ""},
}Let's verify if we've covered all possible value types:
✅ Verification successful
Let me analyze the output to verify if all value types are covered in the test cases.
From the search results in server/pkg/value, I can find these type constants:
-
Basic types:
- TypeText, TypeTextArea, TypeRichText, TypeMarkdown ✓
- TypeSelect, TypeTag ✓
- TypeInteger, TypeNumber ✓
- TypeBool, TypeCheckbox ✗ (TypeCheckbox missing in tests)
- TypeDateTime ✓
- TypeURL ✓
-
Complex types:
- TypeAsset ✓
- TypeGroup ✓
- TypeReference ✓
- TypeGeometryObject, TypeGeometryEditor ✓
-
Special types:
- TypeUnknown (handled as "unknown" in tests) ✓
Test cases are missing TypeCheckbox value type
The test cases cover most value types except for TypeCheckbox. However, looking at the registry implementation in the output:
server/pkg/value/registry.go: TypeCheckbox: &propertyBool{},We can see that TypeCheckbox uses the same property handler as TypeBool, so it would be mapped to "boolean" in the schema anyway. Therefore, this is not a critical issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all value.Type constants are covered in the test
# Expected: All value.Type constants should be present in the test file
# Search for Type constants in the value package
rg "Type[A-Z][a-zA-Z]+" "server/pkg/value" --type go
# Compare with test cases
rg "value\.Type[A-Z][a-zA-Z]+" "server/internal/adapter/publicapi/schema_export_test.go"
Length of output: 20849
server/internal/adapter/integration/schema_export_test.go (3)
1-8: LGTM! Clean and minimal imports.
The package declaration and imports are appropriate for an integration test file.
35-42: Verify the implementation of determineTypeAndFormat function.
The test execution logic is clean and follows best practices. However, we need to ensure that the determineTypeAndFormat function being tested is properly implemented.
Let's verify the implementation:
✅ Verification successful
The search results show that the determineTypeAndFormat function is properly implemented in both server/internal/adapter/integration/schema_export.go and server/internal/adapter/publicapi/schema_export.go. Let me verify which one is being tested by checking the test file's package and imports.
Function implementation is properly defined and tested
The determineTypeAndFormat function is correctly implemented in the same package (integration) as the test file. The test is comprehensive, covering all value types including edge cases, and the function implementation in schema_export.go matches the expected behavior defined in the test cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the determineTypeAndFormat function implementation
rg -t go "func determineTypeAndFormat" --context 10
Length of output: 3390
Script:
#!/bin/bash
# Check the package and imports of the test file
cat server/internal/adapter/integration/schema_export_test.go
Length of output: 1192
1-42: Verify alignment with OpenAPI specification.
The test covers various data types and formats. Let's ensure these types align with the OpenAPI specification for the schema export endpoints.
✅ Verification successful
Let me search for the actual schema export implementation to verify the type mappings.
Based on the implementation found in the codebase, I can now generate the final response:
Type mappings are correctly aligned with OpenAPI specification
The test's type and format mappings in determineTypeAndFormat function align perfectly with OpenAPI 3.0.3 specification:
- Basic types (string, integer, number, boolean) match OpenAPI's primitive types
- Date-time format uses the standard "date-time" format string
- URI format uses the standard "uri" format string
- Binary data uses the standard "binary" format string
- Object and array types follow OpenAPI's schema object specification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for OpenAPI specifications related to schema export
rg -t yaml "openapi|swagger" --context 10
# Look for schema-related type definitions in OpenAPI specs
rg -t yaml "schema.*type:|format:" --context 5
Length of output: 18097
Script:
#!/bin/bash
# Search for the determineTypeAndFormat function implementation
ast-grep --pattern 'func determineTypeAndFormat($_, $_) ($_, $_) {
$$$
}'
# Search for any other schema export related code
rg "determineTypeAndFormat|SchemaJSON|OpenAPI" -A 5
Length of output: 16043
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
server/internal/adapter/publicapi/schema_export_test.go (3)
19-126: Consider breaking down the test into smaller, focused test cases.While the test is comprehensive in covering various field types, it could be more maintainable and debuggable if split into separate sub-tests using
t.Run()for each field type.Example refactor:
func TestBuildProperties(t *testing.T) { - ctx := context.Background() - r := memory.New() - // ... (setup code) ... - - fieldList := schema.FieldList{sf1, sf2, sf3, sf4, sf5, sf6} - properties := buildProperties(uc, fieldList, ctx) - assert.Equal(t, expectedProperties, properties) + tests := []struct { + name string + field *schema.Field + want map[string]interface{} + }{ + { + name: "text field", + field: createTextField(), + want: map[string]interface{}{ + "type": "string", + "maxLength": 100, + // ... + }, + }, + // ... more test cases ... + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + r := memory.New() + uc := &interfaces.Container{Schema: interactor.NewSchema(r, nil)} + + properties := buildProperties(uc, schema.FieldList{tt.field}, ctx) + assert.Equal(t, tt.want, properties) + }) + } }
27-37: Consider extracting magic numbers into named constants.The test uses magic numbers like
100for text length limits. Consider extracting these into named constants for better maintainability and clarity.Example:
+const ( + defaultMaxTextLength = 100 + defaultMinNumber = 1 + defaultMaxNumber = 100 +) -sf1 := schema.NewField(schema.NewText(lo.ToPtr(100)).TypeProperty())... +sf1 := schema.NewField(schema.NewText(lo.ToPtr(defaultMaxTextLength)).TypeProperty())...
128-160: LGTM! Well-structured table-driven test.The test provides excellent coverage of all value types and their mappings. Consider adding comments to document the relationship between CMS value types and OpenAPI types/formats for better maintainability.
Example documentation:
func TestDetermineTypeAndFormat(t *testing.T) { + // Test cases mapping CMS value types to OpenAPI schema types and formats + // Reference: https://swagger.io/docs/specification/data-models/data-types/ tests := []struct { input value.Type wantType string wantFmt string }{server/internal/adapter/integration/schema_export_test.go (2)
19-126: Consider refactoring for better maintainability.While the test is comprehensive and covers various field types, consider these improvements:
- Break down the test setup into helper functions to improve readability
- Add test cases for error scenarios (e.g., invalid field configurations)
- Consider using table-driven tests for different field type combinations
Example refactor:
+func setupTestFields(t *testing.T) schema.FieldList { + // Move field creation logic here + return fieldList +} +func getExpectedProperties() *map[string]interface{} { + // Move expected properties here + return expectedProperties +} func TestBuildProperties(t *testing.T) { ctx := context.Background() r := memory.New() uc := &interfaces.Container{Schema: interactor.NewSchema(r, nil)} - // Current field creation logic... + fieldList := setupTestFields(t) - // Current expected properties... + expectedProperties := getExpectedProperties() properties := buildProperties(uc, fieldList, ctx) assert.Equal(t, expectedProperties, properties) }
128-160: Well-structured table-driven tests with room for enhancement.The test demonstrates excellent use of table-driven tests and comprehensive type coverage. Consider these minor enhancements:
- Document the rationale for defaulting unknown types to "string"
- Add test cases for edge cases (empty/nil input)
Example enhancement:
func TestDetermineTypeAndFormat(t *testing.T) { tests := []struct { input value.Type wantType string wantFmt string }{ // ... existing cases ... + // Edge cases + {value.Type(""), "string", ""}, // Empty type + // Document unknown type behavior + {value.Type("unknown"), "string", ""}, // Unknown types default to string for backward compatibility } // ... rest of the test }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
server/internal/adapter/integration/schema_export_test.go(1 hunks)server/internal/adapter/publicapi/schema_export_test.go(1 hunks)
🔇 Additional comments (2)
server/internal/adapter/publicapi/schema_export_test.go (1)
1-17: LGTM! Package and imports are well-organized.
The imports are clean and necessary for the test implementation.
server/internal/adapter/integration/schema_export_test.go (1)
1-17: LGTM! Well-organized imports.
The imports are clean, necessary, and properly organized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
server/internal/adapter/integration/schema_export.go (2)
18-27: Consider adding validation for schema propertiesWhile the implementation follows JSON Schema standards, consider adding validation for required properties and ensuring that the properties map is not nil before creating the schema.
func NewSchemaJSON(id string, title, description *string, pp map[string]interface{}) integrationapi.SchemaJSON { + if pp == nil { + pp = make(map[string]interface{}) + } return integrationapi.SchemaJSON{ Schema: defaultJSONSchemaVersion, Id: id, Title: title, Description: description, Type: "object", Properties: pp, } }
277-300: Add error handling for unknown typesThe
determineTypeAndFormatfunction defaults to "string" format for unknown types without logging or warning. Consider adding error handling or logging for unexpected types.func determineTypeAndFormat(t value.Type) (string, string) { switch t { case value.TypeText, value.TypeTextArea, value.TypeRichText, value.TypeMarkdown, value.TypeSelect, value.TypeTag, value.TypeReference: return "string", "" // ... other cases ... default: + log.Printf("Warning: Unknown type %v defaulting to string", t) return "string", "" } }server/schemas/integration.yml (2)
1832-1848: Consider adding additional JSON Schema fields for better schema documentation.The
schemaJSONdefinition could benefit from additional standard JSON Schema fields:
definitionsfor reusable schema definitionsrequiredarray for required propertiesadditionalItemsfor array type controlexamplesfor property usage examplesschemaJSON: type: object required: ["$schema", "$id", "type", "properties"] properties: $schema: type: string $id: type: string title: type: string description: type: string type: type: string properties: type: object additionalProperties: true + definitions: + type: object + additionalProperties: true + required: + type: array + items: + type: string + examples: + type: array + items: {}
178-184: Consider adding rate limiting and cache control headers.For better API management and performance, consider adding:
- Rate limiting headers:
X-RateLimit-LimitX-RateLimit-RemainingX-RateLimit-Reset- Cache control headers:
Cache-ControlETagLast-ModifiedThis would help clients better manage their requests and cache responses effectively.
Example addition to the 200 response:
'200': description: A JSON object content: application/json: schema: $ref: '#/components/schemas/schemaJSON' + headers: + X-RateLimit-Limit: + schema: + type: integer + X-RateLimit-Remaining: + schema: + type: integer + X-RateLimit-Reset: + schema: + type: integer + format: unix-timestamp + Cache-Control: + schema: + type: string + ETag: + schema: + type: string + Last-Modified: + schema: + type: string + format: date-timeAlso applies to: 207-213, 315-321, 1055-1061, 1084-1090
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
server/internal/adapter/integration/schema_export.go(1 hunks)server/internal/adapter/integration/server.gen.go(21 hunks)server/internal/adapter/publicapi/schema_export.go(1 hunks)server/internal/adapter/publicapi/schema_export_test.go(1 hunks)server/internal/adapter/publicapi/types.go(1 hunks)server/pkg/integrationapi/types.gen.go(1 hunks)server/schemas/integration.yml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- server/internal/adapter/publicapi/schema_export.go
- server/internal/adapter/publicapi/schema_export_test.go
- server/internal/adapter/publicapi/types.go
- server/pkg/integrationapi/types.gen.go
🔇 Additional comments (10)
server/internal/adapter/integration/schema_export.go (4)
1-14: LGTM: Clean package structure and imports
The package structure and imports are well-organized and appropriate for the functionality being implemented.
29-211: Referencing previous code duplication comment
The previous review comment about code duplication in handler methods is still valid and should be addressed.
265-269: Referencing previous circular reference comment
The previous review comment about protecting against circular group references is still valid and should be addressed.
69-70:
Fix incorrect model ID usage
The function uses request.ModelId instead of m.ID() which was already retrieved. This could cause issues if the model ID in the request doesn't match the found model's ID.
- sp, err := uc.Schema.FindByModel(ctx, request.ModelId, op)
+ sp, err := uc.Schema.FindByModel(ctx, m.ID(), op)Likely invalid or redundant comment.
server/schemas/integration.yml (1)
165-192: LGTM! The schema export endpoints are well-structured.
The new endpoints provide comprehensive access patterns for schema export with consistent:
- Security (bearer auth)
- Error handling
- Response structures
- Parameter validation
Note: The format: binary specification issue was already identified in past reviews and should be addressed.
Also applies to: 193-221, 302-329, 1041-1069, 1070-1098
server/internal/adapter/integration/server.gen.go (5)
Line range hint 93-167: New schema export endpoints added to ServerInterface
The interface has been extended with 6 new methods for JSON schema export functionality:
- MetadataSchemaByModelAsJSON
- SchemaByModelAsJSON
- MetadataSchemaByModelWithProjectAsJSON
- SchemaByModelWithProjectAsJSON
- SchemaByIDWithProjectAsJSON
- SchemaByIDAsJSON
The endpoints follow RESTful conventions and are consistently named.
697-731: Handler implementations follow consistent error handling pattern
The generated handlers follow the standard pattern:
- Parameter binding from URL/context
- Error handling for invalid parameters
- Setting bearer auth scopes
- Invoking the actual handler implementation
- Converting and returning the response
The error handling is comprehensive and consistent across all handlers.
Also applies to: 1167-1217, 1260-1284, 1447-1463
Line range hint 1548-1572: Routes registered with proper HTTP methods
The new endpoints are registered with GET method which is appropriate for retrieving schema data. The routes follow REST conventions:
GET /models/:modelId/metadata_schema.json
GET /models/:modelId/schema.json
GET /projects/:projectIdOrAlias/models/:modelIdOrKey/metadata_schema.json
GET /projects/:projectIdOrAlias/models/:modelIdOrKey/schema.json
GET /projects/:projectIdOrAlias/schemata/:schemaId/schema.json
GET /schemata/:schemaId/schema.json
2501-2595: Request/Response types properly defined
Each endpoint has corresponding request and response types with:
- Proper parameter types (ModelIdParam, ProjectIdOrAliasParam etc.)
- Response interfaces with Visit methods
- HTTP status code specific response types (200, 400, 401, 404, 500)
- Content-Type headers set correctly to "application/json"
The type definitions are complete and follow the standard pattern.
Also applies to: 3155-3251, 3307-3354, 3593-3639
5186-5257: OpenAPI spec encoded and compressed properly
The swagger specification is:
- Base64 encoded
- Gzip compressed
- JSON marshaled
- Split into manageable string chunks
The spec includes all the new endpoint definitions and follows OpenAPI standards.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
server/pkg/exporters/schema_json_test.go (3)
16-66: Consider improving test setup maintainabilityThe test setup is comprehensive but could benefit from the following improvements:
- Extract field creation into helper functions to reduce duplication and improve readability
- Add more descriptive comments explaining the test scenarios
- Define constants for magic numbers (e.g.,
100for text length)Example refactor for field creation:
func createTextField(t *testing.T) *schema.Field { const maxLength = 100 fId := id.NewFieldID() key := id.RandomKey() return schema.NewField(schema.NewText(lo.ToPtr(maxLength)).TypeProperty()). ID(fId). Key(key). MustBuild() }
122-154: Improve test organization and coverageThe table-driven tests are well-structured, but consider these improvements:
- Group similar types together (e.g., all string-based types)
- Add test cases for custom types
- Use more descriptive test names instead of just the type
Example refactor:
func TestDetermineTypeAndFormat(t *testing.T) { tests := []struct { name string input value.Type wantType string wantFmt string }{ // String-based types { name: "plain text should return string type without format", input: value.TypeText, wantType: "string", wantFmt: "", }, // ... more grouped test cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { gotType, gotFmt := determineTypeAndFormat(tt.input) assert.Equal(t, tt.wantType, gotType) assert.Equal(t, tt.wantFmt, gotFmt) }) } }Also, consider adding test cases for:
- Custom field types
- Invalid/deprecated types
- Type-specific validation rules
1-2: Add package documentationConsider adding package documentation to explain the purpose of the exporters package and its JSON schema functionality.
Example:
// Package exporters provides functionality for exporting CMS schemas and data // in various formats. The JSON schema export functionality implements the // JSON Schema specification for describing the structure of schema fields. package exportersserver/internal/adapter/integration/schema_export.go (1)
16-202: Consider adding middleware for common operationsAll handlers share common patterns for context extraction, operator retrieval, and error handling. Consider implementing middleware to handle these cross-cutting concerns.
Example middleware implementation:
func withContextOperator(next func(ctx context.Context, op *usecase.Operator) error) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() op := adapter.Operator(ctx) if err := next(ctx, op); err != nil { // Handle error based on type handleError(w, err) return } } }server/pkg/exporters/schema_json.go (1)
37-41: Includerequiredfields in the schemaThe generated schema doesn't specify which fields are required. In JSON Schema, specifying a
requiredarray enhances validation by indicating mandatory fields. Consider adding logic to identify and include required fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
server/internal/adapter/integration/schema_export.go(1 hunks)server/internal/adapter/publicapi/schema_export.go(1 hunks)server/pkg/exporters/schema_json.go(1 hunks)server/pkg/exporters/schema_json_test.go(1 hunks)
🔇 Additional comments (5)
server/internal/adapter/publicapi/schema_export.go (3)
37-46: LGTM!
The mapping function is clean and correctly transfers all fields between types.
21-24:
Fix potential nil pointer dereference when accessing m.Public().
The error check needs to be separated from the public check to prevent nil pointer dereference.
Apply this diff:
m, err := c.usecases.Model.FindByIDOrKey(ctx, pr.ID(), model.IDOrKey(mKey), nil)
-if err != nil || !m.Public() {
+if err != nil {
return SchemaJSON{}, rerror.ErrNotFound
+}
+if !m.Public() {
+ return SchemaJSON{}, rerror.ErrNotFound
}26-29:
Ensure sp is not nil before proceeding.
The schema package could be nil even if no error occurred.
Apply this diff:
sp, err := c.usecases.Schema.FindByModel(ctx, m.ID(), nil)
-if err != nil {
+if err != nil || sp == nil {
return SchemaJSON{}, rerror.ErrNotFound
}server/internal/adapter/integration/schema_export.go (1)
57-58:
Use consistent model ID reference
The code uses request.ModelId instead of m.ID() which was already retrieved. This could cause issues if the model ID in the request doesn't match the found model's ID.
Apply this diff to fix the issue:
-sp, err := uc.Schema.FindByModel(ctx, request.ModelId, op)
+sp, err := uc.Schema.FindByModel(ctx, m.ID(), op)Likely invalid or redundant comment.
server/pkg/exporters/schema_json.go (1)
98-121: Review type and format mappings in determineTypeAndFormat
The determineTypeAndFormat function effectively maps internal types to JSON Schema types and formats. Ensure all custom types are appropriately handled or defaulted, and consider any future types that might need mapping.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
server/internal/adapter/publicapi/schema_export.go (2)
33-42: Add function documentation.The
toSchemaJSONfunction would benefit from documentation explaining its purpose and the relationship between the source and target types.Add documentation like this:
+// toSchemaJSON converts an internal exporters.SchemaJSON to the public API SchemaJSON type. +// This conversion ensures we maintain a clear boundary between internal and public types. func toSchemaJSON(s exporters.SchemaJSON) SchemaJSON {
12-31: Consider architectural improvements.A few suggestions to enhance the implementation:
- Consider using more specific error types instead of generic
rerror.ErrNotFoundto help clients distinguish between different error cases (e.g., project not found vs. model not found vs. schema not found).- Add input validation for
pKeyandmKeyto ensure they meet any format requirements before making database calls.Would you like me to provide an example implementation of these improvements?
server/internal/adapter/integration/schema_export.go (1)
18-23: Enhance error messages for better debuggingWhile the error handling correctly distinguishes between different response types, the error messages could be more descriptive to aid in debugging and client understanding.
Consider wrapping errors with additional context:
m, err := uc.Model.FindByID(ctx, request.ModelId, op) if err != nil { if errors.Is(err, rerror.ErrNotFound) { - return SchemaByModelAsJSON404Response{}, err + return SchemaByModelAsJSON404Response{}, fmt.Errorf("model not found: %s: %w", request.ModelId, err) } - return SchemaByModelAsJSON400Response{}, err + return SchemaByModelAsJSON400Response{}, fmt.Errorf("failed to retrieve model %s: %w", request.ModelId, err) }Also applies to: 47-52, 82-88, 122-128, 153-159, 191-197
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
server/internal/adapter/integration/schema_export.go(1 hunks)server/internal/adapter/publicapi/schema_export.go(1 hunks)server/pkg/exporters/schema_json.go(1 hunks)server/pkg/exporters/schema_json_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/pkg/exporters/schema_json.go
- server/pkg/exporters/schema_json_test.go
🔇 Additional comments (3)
server/internal/adapter/publicapi/schema_export.go (1)
18-21:
Fix potential nil pointer dereference when checking model.
The current error handling could lead to a nil pointer dereference if m is nil. The error check should be separated from the public check.
Apply this diff to fix the issue:
m, err := c.usecases.Model.FindByIDOrKey(ctx, pr.ID(), model.IDOrKey(mKey), nil)
-if err != nil || !m.Public() {
+if err != nil {
+ return SchemaJSON{}, rerror.ErrNotFound
+}
+if !m.Public() {
return SchemaJSON{}, rerror.ErrNotFound
}Likely invalid or redundant comment.
server/internal/adapter/integration/schema_export.go (2)
13-215: 🛠️ Refactor suggestion
Reduce code duplication through helper functions
The handler methods share significant code structure in:
- Context and operator retrieval
- Error handling patterns
- Response construction
Consider extracting common logic into helper functions:
type schemaResponse interface {
SchemaByModelAsJSON200JSONResponse |
MetadataSchemaByModelAsJSON200JSONResponse |
// ... other response types
}
func buildSchemaResponse[T schemaResponse](
ctx context.Context,
uc *usecases.Container,
modelFn func() (*model.Model, error),
schemaFn func(*model.Model) (*schema.Schema, error),
) (T, error) {
op := adapter.Operator(ctx)
m, err := modelFn()
if err != nil {
// Handle error...
}
sp, err := schemaFn(m)
if err != nil {
// Handle error...
}
// Build and return response...
}This would simplify the handlers to:
func (s *Server) SchemaByModelAsJSON(ctx context.Context, request SchemaByModelAsJSONRequestObject) (SchemaByModelAsJSONResponseObject, error) {
return buildSchemaResponse(ctx, adapter.Usecases(ctx),
func() (*model.Model, error) {
return uc.Model.FindByID(ctx, request.ModelId, op)
},
func(m *model.Model) (*schema.Schema, error) {
return uc.Schema.FindByModel(ctx, m.ID(), op)
},
)
}54-54:
Fix inconsistent model ID usage
The method uses request.ModelId instead of m.ID() for schema lookup, which could cause issues if the model ID in the request doesn't match the found model's ID.
Apply this diff to fix the issue:
- sp, err := uc.Schema.FindByModel(ctx, request.ModelId, op)
+ sp, err := uc.Schema.FindByModel(ctx, m.ID(), op)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
server/internal/adapter/publicapi/schema_export.go (1)
44-59: Optimize map allocation with known sizeConsider pre-allocating the result map with the known size of the input map for better performance.
func toSchemaJSONProperties(pp map[string]exporters.SchemaJSONProperties) map[string]SchemaJSONProperties { - res := map[string]SchemaJSONProperties{} + res := make(map[string]SchemaJSONProperties, len(pp)) for k, v := range pp { res[k] = SchemaJSONProperties{server/pkg/exporters/schema_json.go (1)
176-199: Add test cases for determineTypeAndFormat functionThe type mapping function handles critical conversions but lacks test coverage for edge cases.
Would you like me to help generate comprehensive test cases for this function?
server/internal/adapter/integration/schema_export.go (1)
218-243: Add input validation for property conversionThe helper functions
toSchemaJSONPropertiesandtoSchemaJSONItemslack input validation for property values, which could lead to invalid schema generation.Consider adding validation:
func toSchemaJSONProperties(pp map[string]exporters.SchemaJSONProperties) map[string]integrationapi.SchemaJSONProperties { res := map[string]integrationapi.SchemaJSONProperties{} for k, v := range pp { + if k == "" { + continue // Skip empty keys + } + // Validate numeric constraints + if v.Minimum != nil && v.Maximum != nil && *v.Minimum > *v.Maximum { + continue // Skip invalid range + } res[k] = integrationapi.SchemaJSONProperties{ Type: v.Type, Title: v.Title, Description: v.Description, Format: v.Format, Minimum: v.Minimum, Maximum: v.Maximum, MaxLength: v.MaxLength, Items: toSchemaJSONItems(v.Items), } } return res }server/schemas/integration.yml (1)
1832-1870: Consider adding additional JSON Schema validation constraintsThe schema definitions are well-structured but could benefit from additional validation constraints to ensure data quality:
For
propertiesinschemaJSON:
- Consider adding
minPropertiesto ensure at least one property is defined- Consider adding
propertyNamespattern to enforce naming conventionsFor
schemaJSONProperties:
- Consider adding
enumfor allowed types- Consider adding
patternfor string formats- Consider adding validation for numeric ranges
Example enhancement:
schemaJSON: type: object required: ["type", "properties"] properties: type: type: string + enum: ["object"] properties: type: object + minProperties: 1 + propertyNames: + pattern: "^[a-zA-Z][a-zA-Z0-9_]*$" additionalProperties: $ref: '#/components/schemas/schemaJSONProperties'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
server/internal/adapter/integration/schema_export.go(1 hunks)server/internal/adapter/integration/server.gen.go(21 hunks)server/internal/adapter/publicapi/schema_export.go(1 hunks)server/internal/adapter/publicapi/types.go(1 hunks)server/pkg/exporters/schema_json.go(1 hunks)server/pkg/exporters/schema_json_test.go(1 hunks)server/pkg/integrationapi/types.gen.go(1 hunks)server/schemas/integration.yml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/internal/adapter/publicapi/types.go
- server/pkg/exporters/schema_json_test.go
🔇 Additional comments (19)
server/internal/adapter/publicapi/schema_export.go (3)
33-42: LGTM: Clean and straightforward conversion function
The function correctly maps all fields from exporters.SchemaJSON to SchemaJSON.
61-69: LGTM: Proper nil handling
The function correctly handles nil input and properly converts the schema JSON items.
3-10: 🛠️ Refactor suggestion
Add missing errors import
Add the errors package to support the error handling improvements suggested above.
import (
"context"
+ "errors"
"github.com/reearth/reearth-cms/server/pkg/exporters"Likely invalid or redundant comment.
server/pkg/exporters/schema_json.go (3)
1-8: LGTM: Package structure and imports are well organized
The package name and imports are appropriate for the functionality being implemented.
10-30: LGTM: Type definitions comply with JSON Schema specification
The struct definitions correctly use $schema and $id fields and follow JSON Schema draft 2020-12 specifications.
Let's verify the JSON Schema compliance:
✅ Verification successful
LGTM: JSON Schema field prefixes are correctly implemented
The verification shows that:
- The
SchemaJSONstruct inserver/pkg/exporters/schema_json.gocorrectly uses$idand$schemaprefixes for JSON Schema specific fields - Other occurrences of "id" and "schema" in the codebase are regular entity identifiers and references, not JSON Schema keywords, so they don't require the
$prefix - The implementation consistently uses the correct JSON Schema draft version (2020-12)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify JSON Schema field names against the specification
# Check if any other fields should use $ prefix according to the spec
rg -g '*.go' '"schema"|"id"' | grep 'json:"[^$]'
Length of output: 3310
Script:
#!/bin/bash
# Let's check the actual usage context of these fields to determine if they are JSON Schema fields or regular entity IDs
# First, let's check the schema-related files
rg -g '*.go' -B 5 -A 5 'json:"schema"'
# Then, let's verify if there are any other JSON Schema related fields that might need the $ prefix
rg -g '*.go' 'json-schema.org' -B 5 -A 5
Length of output: 12817
94-101:
Fix Group field handling to comply with JSON Schema array specification
The current implementation doesn't properly structure array items according to JSON Schema. For array types, we need to specify both the array type and the structure of its items.
Apply this fix:
Group: func(f *schema.FieldGroup) {
if gsMap != nil {
gs := gsMap[f.Group()]
if gs != nil {
- fieldSchema.Items = BuildItems(gs.Fields())
+ fieldSchema.Type = "array"
+ fieldSchema.Items = &SchemaJSON{
+ Type: "object",
+ Properties: BuildProperties(gs.Fields(), nil),
+ }
}
}
},Likely invalid or redundant comment.
server/internal/adapter/integration/schema_export.go (1)
55-55:
Fix incorrect model ID usage in schema lookup
The function uses request.ModelId instead of m.ID() which was already retrieved. This could cause issues if the model ID in the request doesn't match the found model's ID.
Apply this diff to fix the issue:
- sp, err := uc.Schema.FindByModel(ctx, request.ModelId, op)
+ sp, err := uc.Schema.FindByModel(ctx, m.ID(), op)Likely invalid or redundant comment.
server/pkg/integrationapi/types.gen.go (3)
554-562: LGTM! The SchemaJSON type follows JSON Schema specification.
The type definition correctly implements the JSON Schema structure with appropriate field types and required/optional markers. The required fields (properties and type) are non-pointer types, while optional fields are pointer types.
564-574: LGTM! The SchemaJSONProperties type follows JSON Schema property specification.
The type definition correctly implements JSON Schema property attributes with:
- Optional fields as pointers (description, format, etc.)
- Required
typefield as non-pointer - Support for nested schemas via the
itemsfield
Line range hint 1-3: Verify the OpenAPI specification changes.
This is a generated file from oapi-codegen. Please ensure that:
- The corresponding OpenAPI specification has been updated with these new types
- The changes were generated using oapi-codegen v2.4.1
- No manual modifications were made to this file
server/schemas/integration.yml (4)
184-184: Remove incorrect binary format from JSON responses
The format: binary specification is incorrect for JSON responses. This format is typically used for binary data like files or images, not for JSON responses.
Remove the format: binary line from all schema JSON response definitions:
schema:
$ref: '#/components/schemas/schemaJSON'
- format: binaryAlso applies to: 213-213, 321-321, 349-349
Line range hint 165-357: LGTM! Well-structured endpoints with proper security and error handling.
The new endpoints for schema export follow RESTful conventions and include proper security requirements, parameters, and comprehensive error responses.
1061-1061: Remove incorrect binary format from JSON responses
The format: binary specification is incorrect for JSON responses in these endpoints as well.
Remove the format: binary line from all schema JSON response definitions:
schema:
$ref: '#/components/schemas/schemaJSON'
- format: binaryAlso applies to: 1090-1090
1041-1098: LGTM! Well-structured project-scoped endpoints.
The additional endpoints for project-scoped schema export are properly implemented with appropriate security and error handling.
server/internal/adapter/integration/server.gen.go (5)
93-98: Interface additions look good and follow consistent patterns
The new interface methods for schema JSON export are well-structured and follow the existing patterns:
- Clear method names indicating their purpose (e.g., MetadataSchemaByModelAsJSON)
- Consistent parameter patterns (modelId, projectId, etc.)
- Proper error handling with return types
Also applies to: 135-140, 144-146, 165-167
2501-2596: Request/Response types are properly defined with appropriate validation
The new types follow best practices:
- Proper request object definitions with required parameters
- Response objects implementing visitor pattern for content negotiation
- Consistent error response types (400, 401, 404, 500)
- JSON content type headers properly set
Also applies to: 3155-3252, 3307-3354, 3593-3640
697-713: Handler implementations are consistent and handle errors appropriately
The handler implementations:
- Properly bind path parameters
- Set authentication scopes
- Follow consistent error handling patterns
- Return appropriate response types
Also applies to: 715-731, 1167-1191, 1193-1217, 1260-1284, 1447-1463
1548-1549: Router registrations are correctly configured
The new routes are properly registered with:
- Correct HTTP methods (GET)
- Proper URL patterns following REST conventions
- Appropriate handler mappings
Also applies to: 1562-1563, 1565-1565, 1572-1572
5186-5258: Verify the OpenAPI spec encoding
The base64 encoded swagger spec has been updated to include the new endpoints. However, it would be good to verify that the encoded spec matches the source OpenAPI definition.
Overview
This PR introduces a feature for exporting schemas in JSON format.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests