feat(server): support float fields #1263
Conversation
WalkthroughThe pull request introduces enhancements to the integration tests for schema and field APIs in the Changes
Possibly related PRs
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: 9
🧹 Outside diff range and nitpick comments (1)
server/internal/adapter/gql/gqlmodel/convert_schema.go (1)
559-580: LGTM! Consider a minor improvement for consistency.The implementation of the
SchemaFieldTypeNumbercase looks correct and consistent with the existing pattern. Good job on maintaining the structure and error handling similar to other cases.For consistency with the
SchemaFieldTypeIntegercase, consider renaming the variabletpitotpn(type property number) in theSchemaFieldTypeNumbercase. This small change would make the variable naming more consistent across different field types.- tpi, err2 := schema.NewNumber(min, max) - if err2 != nil { - err = err2 - } - tpRes = tpi.TypeProperty() + tpn, err2 := schema.NewNumber(min, max) + if err2 != nil { + err = err2 + } + tpRes = tpn.TypeProperty()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- server/e2e/gql_field_test.go (4 hunks)
- server/e2e/gql_item_test.go (3 hunks)
- server/e2e/integration_item_test.go (3 hunks)
- server/internal/adapter/gql/generated.go (17 hunks)
- server/internal/adapter/gql/gqlmodel/convert_schema.go (1 hunks)
- server/internal/adapter/gql/gqlmodel/models_gen.go (5 hunks)
- server/internal/adapter/integration/item_import.go (2 hunks)
- server/internal/adapter/integration/schema.go (1 hunks)
- server/internal/adapter/integration/server.gen.go (1 hunks)
- server/pkg/integrationapi/types.gen.go (1 hunks)
- server/schemas/field.graphql (5 hunks)
- server/schemas/integration.yml (1 hunks)
🧰 Additional context used
🔇 Additional comments (35)
server/schemas/field.graphql (6)
12-12: LGTM: Addition ofNumbertype is appropriate.The inclusion of
Numberin theSchemaFieldTypeenum is a logical extension of the existing types. It provides support for decimal or floating-point numbers, which is distinct from theIntegertype. This addition enhances the flexibility of the schema system.
85-85: LGTM:SchemaFieldNumbercorrectly added to union type.The addition of
SchemaFieldNumberto theSchemaFieldTypePropertyunion is consistent with the existing pattern and necessary for the new number type to be fully integrated into the schema system. This change ensures thatSchemaFieldNumbercan be used whereverSchemaFieldTypePropertyis expected.
243-247: LGTM:SchemaFieldNumberInputis well-defined.The
SchemaFieldNumberInputinput type is correctly structured, mirroring theSchemaFieldNumbertype. It follows the established pattern for input types in the schema and uses appropriate types for each field. This consistency ensures that the new number field type can be easily integrated into existing mutation operations.
297-297: LGTM:numberfield correctly added toSchemaFieldTypePropertyInput.The addition of the
numberfield toSchemaFieldTypePropertyInputis correct and necessary. It allows the new number field type to be used in input operations, maintaining consistency with other field types. The@onlyOnedirective on the input type ensures that only one field type can be specified at a time, which is appropriate for this schema design.
147-151: LGTM with a note on type safety.The
SchemaFieldNumbertype is well-structured and consistent with other field types, particularlySchemaFieldInteger. The use ofFloatforminandmaxis appropriate for handling decimal values.However, please ensure that there's proper validation for the
defaultValueof typeAny. While this provides flexibility, it could potentially lead to type inconsistencies if not carefully managed. Consider implementing runtime checks or a more specific type if possible.To verify the handling of
defaultValue, please run the following script:
Line range hint
1-365: Summary: Comprehensive and consistent implementation ofNumberfield type.The changes in this file successfully introduce support for a new
Numberfield type across the schema. The implementation is thorough, updating all necessary parts of the schema including theSchemaFieldTypeenum,SchemaFieldTypePropertyunion, and corresponding type and input type definitions. The changes follow established patterns in the existing schema, ensuring consistency and ease of integration.As a final step, it would be beneficial to verify that all parts of the system that interact with schema fields (e.g., resolvers, validators, UI components) have been updated to handle the new
Numbertype correctly.To perform a final verification, please run the following script:
This will help ensure that the new
Numbertype is properly integrated throughout the system.server/internal/adapter/integration/schema.go (2)
395-397: LGTM! Enhancement to support number fields.The addition of the
ValueTypeNumbercase in theFromSchemaTypePropertyfunction is a valuable enhancement. It allows for the creation of number fields in the schema, which increases the flexibility of the schema definition process. The implementation is consistent with other numeric types and appears to be correct.
395-397: Verify the impact of adding ValueTypeNumber support.The addition of
ValueTypeNumbersupport in theFromSchemaTypePropertyfunction is a significant enhancement. To ensure a smooth integration, please consider the following:
- Update any parts of the codebase that interact with schema fields to handle the new number type appropriately.
- Verify that data validation, storage, and retrieval processes can accommodate number fields.
- Ensure that user interfaces and API endpoints working with schema definitions support the new number type.
To assist with this verification, we can run the following script to identify potential areas that might need updates:
This script will help identify areas of the codebase that might need to be updated to fully support the new
ValueTypeNumber. Would you like me to provide more detailed analysis based on the script results?✅ Verification successful
To further investigate the impact of adding ValueTypeNumber support, we need to search for its specific usage and related number field handling in the codebase. Let's run the following script:
This script will help us identify if ValueTypeNumber is already being used, how number fields are currently handled, and if there are any switch statements that might need to be updated to include the new type.
To gather the final pieces of information needed for our analysis, let's run the following script:
This script will provide more context around ValueTypeNumber usage, help us find type-related logic, and show how FieldNumber is used in the codebase.
The addition of ValueTypeNumber support in the FromSchemaTypeProperty function appears to be properly implemented and integrated into the existing system. After analyzing the codebase, we can conclude that:
- ValueTypeNumber is defined and used consistently in the integration API and schema handling.
- The implementation of FieldNumber and related structures is extensive and follows the patterns established for other field types.
- The new type is properly handled in type assertions and switch statements where field types are concerned.
- There are no apparent inconsistencies in how number fields are handled compared to integer fields.
The impact of this change seems to be well-contained within the schema and field handling parts of the codebase, which is exactly what we would expect for this kind of addition.
Given these findings, it appears that the addition of ValueTypeNumber support has been implemented correctly and should not cause any unexpected issues in the rest of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Identify areas in the codebase that might need updates to support ValueTypeNumber # Search for uses of ValueTypeInteger as a proxy for number-related logic echo "Searching for ValueTypeInteger usage:" rg --type go "ValueTypeInteger" # Search for switch statements on ValueType to ensure they include the new ValueTypeNumber echo "\nSearching for switch statements on ValueType:" rg --type go "switch.*ValueType" # Search for schema-related functions that might need to handle the new number type echo "\nSearching for schema-related functions:" rg --type go "func.*(Schema|Field|Value)"Length of output: 419907
Script:
#!/bin/bash echo "Searching for ValueTypeNumber usage:" rg --type go "ValueTypeNumber" echo "\nSearching for number field handling:" rg --type go "FieldNumber|NumberField" echo "\nSearching for switch statements on field types:" rg --type go "switch.*\bType\b.*\bcase\b"Length of output: 15782
Script:
#!/bin/bash echo "Searching for additional ValueTypeNumber usage:" rg --type go "ValueTypeNumber" -C 3 echo "\nSearching for type assertions related to field types:" rg --type go "\\.(Type|FieldType)\\(\\)" echo "\nSearching for FieldNumber usage:" rg --type go "FieldNumber" -C 2Length of output: 38745
server/e2e/gql_item_test.go (2)
415-415: Update in expected field values reflects new behavior.The change in the expected values for the
fieldsvariable now includes anilvalue for the asset field. This update correctly reflects the new behavior where the asset field can be empty or unset.
435-435: All fields can now be cleared to nil values.The updated assertion for the
fieldsvariable after the update operation now expects all values to benil. This change accurately represents the new functionality allowing all fields to be cleared or reset.server/pkg/integrationapi/types.gen.go (3)
166-166: Impact on existing structs using ValueTypeThe addition of
ValueTypeNumberexpands the possible values for theTypefield in bothSchemaFieldandFieldstructs. While no immediate changes are required in these structs, they can now accept the newValueTypeNumber. Ensure that any code handling these structs is updated to properly handle the new numeric type.Let's check for any switch statements or type assertions that might need updating:
#!/bin/bash # Search for switch statements or type assertions involving ValueType rg --type go "switch.*ValueType|case.*ValueType|\(ValueType\)" --glob '!server/pkg/integrationapi/types.gen.go'
Line range hint
1-1024: Generated file with minimal changesThis file is generated, and only one constant has been added. While the change itself is correct and doesn't introduce any immediate issues, it's important to note:
- Ensure that the source of this generated file (likely an API specification) has been updated correspondingly.
- Check if any code generation scripts or tools need to be re-run to ensure consistency across the codebase.
- Verify that any documentation referring to
ValueTypeis updated to include the newnumbertype.Let's check for any API specification files that might be the source of this generated code:
#!/bin/bash # Search for OpenAPI/Swagger specification files fd -e yaml -e yml -e json | rg -i "openapi|swagger"
166-166: New constant added for numeric value typeA new constant
ValueTypeNumberhas been added to theValueTypeenumeration. This addition expands the capabilities of the API to handle numeric values explicitly.To ensure this change is consistent with the rest of the codebase, let's check if this new value type is used or referenced elsewhere:
✅ Verification successful
ValueTypeNumberis correctly referenced inserver/internal/adapter/integration/schema.goNo further changes required.
server/internal/adapter/integration/schema.goline:case integrationapi.ValueTypeNumber:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any references to ValueTypeNumber in the codebase rg --type go "ValueTypeNumber|ValueType.*number" --glob '!server/pkg/integrationapi/types.gen.go'Length of output: 271
Script:
#!/bin/bash # Search for any references to ValueTypeNumber in the codebase excluding the types.gen.go file rg --type go "ValueTypeNumber|ValueType.*number" --glob '!server/pkg/integrationapi/types.gen.go'Length of output: 183
server/schemas/integration.yml (1)
1674-1674: LGTM! Verify implementation across the system.The addition of 'number' as a value type enhances the API's capability to handle float fields, which aligns with the PR objective. This change improves the flexibility of the data model.
To ensure this change is properly implemented and doesn't cause any unintended side effects, please run the following verification:
Please review the output of these tests and ensure that:
- The 'number' type is properly defined in schema definitions where needed.
- All switch statements or similar constructs handling value types are updated to include the 'number' type.
- Any hardcoded lists of value types are updated to include 'number'.
server/e2e/integration_item_test.go (2)
1576-1576: LGTM! CSV output format updated correctly.The expected CSV output format has been properly updated to include the new "number" column. This change ensures consistency with the new field structure and maintains the correctness of the CSV export functionality.
1596-1596: LGTM! CSV output format updated consistently.The expected CSV output format has been correctly updated to include the new "number" column. This change maintains consistency with the new field structure and ensures the correctness of the CSV export functionality when exporting items with project information.
server/internal/adapter/gql/generated.go (15)
691-695: LGTM: Addition ofSchemaFieldNumberStructureThe
SchemaFieldNumberstruct is correctly defined with the necessary fieldsDefaultValue,Max, andMin, which will help handle numeric fields in the GraphQL schema.
4012-4032: LGTM: Complexity Calculations forSchemaFieldNumberFieldsThe complexity functions for
SchemaFieldNumberfieldsdefaultValue,max, andminare appropriately added, ensuring proper complexity calculations during query execution.
4788-4788: LGTM: AddingSchemaFieldNumberInputUnmarshalingThe
ec.unmarshalInputSchemaFieldNumberInputfunction is correctly included to handle unmarshaling ofSchemaFieldNumberInput.
5190-5190: LGTM: AddingNumbertoSchemaFieldTypeEnumThe
Numbertype is properly added to theSchemaFieldTypeenum, enhancing the schema to support numeric fields.
5263-5263: LGTM: IncludingSchemaFieldNumberinSchemaFieldTypePropertyUnionThe
SchemaFieldNumbertype is correctly added to theSchemaFieldTypePropertyunion, integrating the numeric field into the GraphQL schema.
5475-5475: LGTM: AddingnumberField toSchemaFieldTypePropertyInputThe
numberfield is appropriately added to theSchemaFieldTypePropertyInput, allowing users to input numeric field configurations.
28748-28870: LGTM: Resolver Functions forSchemaFieldNumberFieldsThe resolver functions
_SchemaFieldNumber_defaultValue,_SchemaFieldNumber_min, and_SchemaFieldNumber_maxare correctly implemented to handle the new fields inSchemaFieldNumber.
38655-38658: LGTM: Includingnumberin Fields Order for UnmarshalingThe
numberfield is correctly added to thefieldsInOrderarray, ensuring it is processed in the correct order during input unmarshaling.
38948-38973: LGTM: Unmarshaling Logic fornumberFieldThe unmarshaling logic for the
numberfield withinunmarshalInputSchemaFieldTypePropertyInputis properly implemented, allowing for correct input handling.
40881-40887: LGTM: HandlingSchemaFieldNumberin Type CastingThe addition of
SchemaFieldNumbercases ensures that objects of this type are correctly handled during type casting.
46836-46875: LGTM: Implementation of_SchemaFieldNumberField ResolverThe
_SchemaFieldNumberresolver function and its implementors are correctly added, enabling proper field resolution forSchemaFieldNumbertypes.
52416-52431: LGTM: Marshal and Unmarshal Functions for Optionalfloat64The
unmarshalOFloat2ᚖfloat64andmarshalOFloat2ᚖfloat64functions are correctly implemented to handle optionalfloat64values, which is essential for fields likeminandmax.
53149-53155: LGTM: Unmarshaling OptionalSchemaFieldNumberInputThe function
unmarshalOSchemaFieldNumberInput2ᚖgqlmodel.SchemaFieldNumberInputis properly added to handle optional inputs forSchemaFieldNumberInput.
5421-5425:⚠️ Potential issueUpdate
defaultValueType inSchemaFieldNumberInputSimilarly, in
SchemaFieldNumberInput, thedefaultValuefield should be changed fromAnytoFloatto maintain consistency and ensure proper type validation.Apply this diff:
input SchemaFieldNumberInput { - defaultValue: Any + defaultValue: Float min: Float max: Float }Likely invalid or redundant comment.
38355-38394:⚠️ Potential issueUpdate Unmarshaling Logic for
defaultValueIf the
defaultValuetype is changed toFloat, ensure that the unmarshaling logic inunmarshalInputSchemaFieldNumberInputis updated accordingly.Apply this diff after updating the schema:
case "defaultValue": ctx := graphql.WithPathContext(ctx, graphql.NewPathWithField("defaultValue")) - data, err := ec.unmarshalOAny2interface(ctx, v) + data, err := ec.unmarshalOFloat2ᚖfloat64(ctx, v) if err != nil { return it, err } it.DefaultValue = dataLikely invalid or redundant comment.
server/internal/adapter/integration/item_import.go (1)
254-256: Enhancement in type assignability logic looks goodThe modifications in the
isAssignablefunction to allowvalue.TypeIntegerto be assigned tovalue.TypeNumberenhance the flexibility of type handling. This change is logically sound and correctly implemented.server/e2e/gql_field_test.go (3)
137-137: LGTMThe addition of
numberFIdto thefIdsstruct is appropriate.
274-274: LGTMIncluding
numberFIdin the assertion ensures the new field is included in the test validations.
291-291: LGTMReturning
numberFIdin thefIdsstruct allows access to the new field in subsequent tests.
| case integrationapi.ValueTypeNumber: | ||
| tpi, _ := schema.NewNumber(nil, nil) | ||
| tpRes = tpi.TypeProperty() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving default value handling and error management.
While the implementation for ValueTypeNumber is correct and consistent with other types, there are a couple of points to consider for future improvements:
-
Default Value: The function signature includes a return value for a default value (
dv *value.Multiple), but it's not being set for most types, including the new Number type. Consider clarifying the intended use of this return value and potentially initializing it appropriately for each type. -
Error Handling: The
NewNumberfunction call ignores a potential error (similar toNewInteger). While this is consistent with the current implementation, it might be worth considering a more robust error handling approach in the future.
Would you like assistance in implementing these improvements?
| @@ -1450,7 +1450,7 @@ func TestIntegrationModelImportJSONWithJsonInput(t *testing.T) { | |||
| mId, _ := createModel(e, pId, "test", "test", "test-1") | |||
| createFieldOfEachType(t, e, mId) | |||
|
|
|||
| jsonContent := `[{"text": "test1", "bool": true, "integer": 1},{"text": "test2", "bool": false, "integer": 2},{"text": "test3", "bool": null, "integer": null}]` | |||
| jsonContent := `[{"text": "test1", "bool": true, "number": 1.1},{"text": "test2", "bool": false, "number": 2},{"text": "test3", "bool": null, "number": null}]` | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Update Remaining "integer" References to "number"
The search revealed multiple instances of "integer" across the codebase. To maintain consistency and support for floating-point numbers, please update all relevant occurrences from "integer" to "number". Ensure that these changes are reflected in:
- Field definitions and type declarations.
- Context paths and API contracts.
- Associated test cases and schemas.
This will help prevent potential type mismatches and ensure seamless functionality across the system.
🔗 Analysis chain
LGTM! Consider verifying the impact of the field type change.
The change from "integer" to "number" in the test JSON content is good and reflects the new support for floating-point numbers. This update aligns the test case with the new field structure.
To ensure this change doesn't have unintended consequences, please run the following script to check for any other occurrences of "integer" fields that might need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of "integer" fields in the codebase
rg '"integer"' --type go
Length of output: 1553
| type SchemaFieldNumber { | ||
| defaultValue: Any | ||
| min: Float | ||
| max: Float | ||
| } |
There was a problem hiding this comment.
Consider Changing defaultValue Type to Float
The defaultValue field in SchemaFieldNumber is currently defined as type Any. For better type safety and clarity, consider changing its type to Float, since it represents a numeric value.
Apply this diff to update the type:
type SchemaFieldNumber {
- defaultValue: Any
+ defaultValue: Float
min: Float
max: Float
}Committable suggestion was skipped due to low confidence.
| @@ -282,7 +286,7 @@ func FieldFrom(k string, v any, sp schema.Package) interfaces.CreateFieldParam { | |||
| case reflect.Uint64: | |||
| case reflect.Float32: | |||
| case reflect.Float64: | |||
| t = value.TypeInteger | |||
| t = value.TypeNumber | |||
There was a problem hiding this comment.
💡 Codebase verification
Impact of Changing value.TypeInteger to value.TypeNumber Confirmed
The mapping of integer types to value.TypeNumber has been identified in multiple parts of the codebase:
- Schema Definitions: Affects how fields are constructed and validated.
- Integration APIs: May alter how integer values are processed and returned.
- Exporters: Changes how data is exported, potentially affecting CSV and GeoJSON outputs.
- Testing: Existing tests are based on
value.TypeIntegerand may need updates to accommodate the new type.
Recommendation:
- Thoroughly review and update all affected areas to ensure compatibility with
value.TypeNumber. - Update existing tests to reflect the new type mappings and validate the changes comprehensively.
🔗 Analysis chain
Verify the impact of mapping integer types to value.TypeNumber
In the FieldFrom function, all integer types are now mapped to value.TypeNumber instead of value.TypeInteger. This change could affect other parts of the codebase that rely on value.TypeInteger. Please ensure this alteration does not introduce unintended side effects.
Run the following script to identify usages of value.TypeInteger and assess potential impacts:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of 'value.TypeInteger' in the codebase to identify areas potentially affected by the change.
# Expected: Review the occurrences to ensure compatibility with the new type mapping.
rg --type go 'value\.TypeInteger'
Length of output: 3052
| numberFId, _ := createField(e, mId, "number", "number", "number", | ||
| false, false, false, false, "Number", | ||
| map[string]any{ | ||
| "number": map[string]any{ | ||
| "defaultValue": nil, | ||
| "min": nil, | ||
| "max": nil, | ||
| }, | ||
| }) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding tests for the "number" field's constraints
While the "number" field is properly added, consider adding tests to validate its defaultValue, min, and max properties to ensure it behaves correctly under various conditions.
| type SchemaFieldNumber struct { | ||
| DefaultValue any `json:"defaultValue,omitempty"` | ||
| Min *float64 `json:"min,omitempty"` | ||
| Max *float64 `json:"max,omitempty"` | ||
| } | ||
|
|
||
| func (SchemaFieldNumber) IsSchemaFieldTypeProperty() {} | ||
|
|
||
| type SchemaFieldNumberInput struct { | ||
| DefaultValue any `json:"defaultValue,omitempty"` | ||
| Min *float64 `json:"min,omitempty"` | ||
| Max *float64 `json:"max,omitempty"` | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid modifying generated code directly
The file models_gen.go is auto-generated by gqlgen, as indicated by the comment // Code generated by github.com/99designs/gqlgen, DO NOT EDIT. at the top of the file. Modifying this file directly is not recommended because your changes will be overwritten the next time the code is generated. Please make your changes to the appropriate GraphQL schema definitions or source files used by the code generator.
| @@ -1130,6 +1144,7 @@ type SchemaFieldTypePropertyInput struct { | |||
| Tag *SchemaFieldTagInput `json:"tag,omitempty"` | |||
| Checkbox *SchemaFieldCheckboxInput `json:"checkbox,omitempty"` | |||
| Integer *SchemaFieldIntegerInput `json:"integer,omitempty"` | |||
| Number *SchemaFieldNumberInput `json:"number,omitempty"` | |||
There was a problem hiding this comment.
Changes to SchemaFieldTypePropertyInput should be made in the schema
The addition of the Number field to SchemaFieldTypePropertyInput should be done in the GraphQL schema definitions, not directly in the generated code. This ensures that the change is preserved and properly reflected when the code is regenerated.
| @@ -2430,6 +2445,7 @@ const ( | |||
| SchemaFieldTypeSelect SchemaFieldType = "Select" | |||
| SchemaFieldTypeTag SchemaFieldType = "Tag" | |||
| SchemaFieldTypeInteger SchemaFieldType = "Integer" | |||
| SchemaFieldTypeNumber SchemaFieldType = "Number" | |||
There was a problem hiding this comment.
Update the GraphQL schema to include SchemaFieldTypeNumber
The new enum value SchemaFieldTypeNumber and its inclusion in AllSchemaFieldType and the IsValid method should be added to the GraphQL schema files, not directly in models_gen.go. Modifying the generated code can lead to inconsistencies and your changes may be lost on regeneration.
Also applies to: 2468-2468, 2479-2479
| "p3H3rbG47ZhgAfdCkhbuxTEDFEYhw/HsSl+dI3aT0DvpqcUziG+u6X0Y5c1UiTbHKqqOQl33szkSZZWN", | ||
| "UVK1cWBAVClQJ4a0qxSFApnEmcp4X1ybEr298CHB0vvwOoDAOKYEEulNDWLdenLzZCM+LgpymH82BshP", | ||
| "eWuezgYCz/bX+N0NZtsIq4nvLFP+Wo3fWbwgp3Zy3iu3Xaaof+LybK2grKGwDRQdVr70xQoc4oxh8aD0", | ||
| "lGbFa0AM2HGmPTm1WkVidbmYdibEQtexMZnQapn5T/iAmJi9O/k8Ds5VHlJ5wMHx5bmcRGr91lH54sLf", | ||
| "Dg4PDk0AR9ACh0fh+4PDg/eh9jkV4LoRko8eTePnUgOVglAqRAdPmBLJTKHKpZ/qmyul+H8dHuryZJ4c", | ||
| "RYtFajz40d9cY7vOnvXL5HuIsuojaAXGdSvFMgp/1+CtNDToyr3tEQjyrtpAR03qud/qWDpf/qjaP6Ce", | ||
| "/L36xi9F24Hko2w+R+xBNZlInOZ9uQJNuVTcasU81MVNUUOPj+qRjYjR2ii7+xiegmhCr9tQXVNML4aM", | ||
| "Sg3XqvZcEaORqVWYlo864pnE/ifd8TOgRLmv75hq07UVX2a2m7TZbuiXzhNGeytCu3r7+4/lDy/L5Aur", | ||
| "8E5xZ2MmisIF5S1scqIcHtNKBVz8H00eNuKRukqWn+blBq7lFpVOzoxVVtt9vtJuax/WalQwo8d8o0G7", | ||
| "8TaM9Gw2vLGQWSW2tYukjK3d5wHX4D+Feolax6/sflEKCYl41sxIXxfJm9dIGgfB8atjUrswh96takr5", | ||
| "GqNHvTunUR/JCPDZ9JCz96eHEsImaH0duofY3VWWojqUd6KN1eBUZIxw++BBXltzKaqjkX6qKt9W0UFN", | ||
| "ORsK5aq2Ju8riYoqVxy/ZHaIwn/7YRLACEoDDuwWWAB6vj7M42GCKvv0I7+7J7BkdrxqtpH7BjZHeU9l", | ||
| "n52kwyUZh80KPrcF3UtUo5VtEKiqXW3PLshnX0lyQb7hVeYWNMWdTeQlwm/i/FdUqje14PDIPrPwmjIL", | ||
| "3RmrQbV0zSs4XLR7aYUSnl6LZ/8kWmXIjILDQvuEgptQeF3sadYlqR2cdNNN+ryS0aMpaDcqItV09mwq", | ||
| "yD2PorMCMtvvn4Gy6yQM8sMCLMnUmrtkDPST1ZhNTbDlAqZBsSfeCP4YX3wJlCca0EmQcWABQXPgbzao", | ||
| "L+jkIXE/Y1E6NKZDWN/IIgNbhbZ2wrrmrpqmwec2JW0crqGSLL4T6qbKERVm9JmGEZ4vKBOrx6Otw6mZ", | ||
| "qLEu5/oVg+aXznvuYrUdS87mH6B/cLWtXL3Q10lnm+1UKuk/tb2LAgkYr57t4nYmC4YETB/Ke8U4sGLz", | ||
| "p/pDXfnRtkejOGvOrMl5gXeThm6uREyM5APvbD9dHXptw3/e43WNCVKnqVT7At8qUgdXVCuukz6T54Rm", | ||
| "JSc8P50qMittHiI9w4b7td2OTfsSCNydDdrabPoQa+Hs4hdWFLbWaYHZ+/Isbsm2nA6tS/VpjxlJgJWP", | ||
| "Fqr1Nf263xKwxQ9NMdd4VcHGHRazYIJTAZJdAkQSfSITJlN/feFMje1d4CoOheoQNpdOyeowvjj8rstg", | ||
| "95C6DuPXLc21jy4duahLeUOYVE3NHvtKfWI5sA4ctkNaHRt49Nh46F9+GmH7QHWWRK608rGH0SYKbB9T", | ||
| "1emcIQqmVYe1Pr0/bF5/X9lcr7L5oqRi/YpCTW3Sb40PYn7bwSKfjP8KxAyJYIaqBhrxwB7P5jfI/Jif", | ||
| "jP/qbZCfyGa2N6gIuBcjg6iC2VoDFh+L6XsBJgqlZoo3q3T7sFU5A10cIis5a2Pt3CAgU6BW0bQIiTm8", | ||
| "cDNBsScg7qywrK+h7dK9gmORW+yifJsi05fJyqagOGFzEJExUshHj6vHTC+NOPWI9vQDNbnlPKYb0OUv", | ||
| "IOzk1+Rp2b2v/0p8/YLjNi6k+E9v325ioDamUKsYOKjYV2N2IQJoTsi1q+vc+1HflVgp5JdXflqqN9eo", | ||
| "bT3oGxazy9xhe9E1/6viCxnJW1OQVYoO3kIwICfsuwleXDfB2kaw+sGdYXoRVtltbwh30RC+hL7/ljaH", | ||
| "3pZ1VCRpn1fGvA6kyvhqB3IbIvQij2V6WrErlak7CF+eVN8JL9Qe/mhlxVQQNpOV0aP70btG39QaN+fL", | ||
| "eknVUCioXoKHmp9Y1i2gtyt6s/6pQsCBj7+e02Npf6j6YciGdnu1pu25MXsd/Dp1cGYdloF18JP27pQZ", | ||
| "ft/Gs90d9vtGmH06oGsjTF54ff7sgIlcyqg4yVshzmvPH9heYLPvvHljnTdVdquTlg2s7tP06DjysG/X", | ||
| "2bfrvKx2nUFtxyai+KTdQCWR3DcG7RuDttYY5Ajo+g1CTyqkmjf01rIWSdRDqzI2NvH/vrNokODJfLJe", | ||
| "P/CGA6iC3ayIjS2v7kZfUe9tQTWyujTH4fbIGpnjynuljdSu2ze358uzM+uZUkDm7Pkd0147cTD++tkb", | ||
| "vbyD6kmwA+RvuuZgAgJ35rRdKcFMAalqcuabMPpmjUQP3L/Ib/DiFOTKGHD7XZEEJihLRXikvmbsfqFU", | ||
| "sAwiX4mD3oC/76P268xPuUO+0yo937odfHPlBh+r2H1xLLJCuQQ0SmKL/Rxli5Qi0yzilbhzzjMpcF//", | ||
| "/KREDQWKTwNBA/1sfjh7jbB9VaNykdtYMQx6LtgnINPSxxcdoxBnjOuvYm7ScLUc+ODNdrDbPkMJ9/5v", | ||
| "lw+gf2q+C6IZ5TWcX1Zh+EbBs4Hk6NF+oXH95iw7Q4/+qn1Bf1/QH6Cpqp6LG9umahuiXn4X1C7SMil1", | ||
| "MA3RwLSicbbXg7TXU3s9NUDj0aPzGd5l7vT2yBDlj6z6saZQso107sCJDHfVnTIs1mHx5Fi2m9LNIX1O", | ||
| "r7D5yS9UnEkncesnYdWzoutiXlqM9VfljmQ8ye7R5XL53wAAAP//H0uKi7KeAAA=", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider externalizing the Swagger specification to reduce binary size
Embedding the Swagger specification as a base64-encoded string directly in the code can significantly increase the binary size and may impact performance. Consider externalizing the Swagger specification into a separate file or using the go:embed directive to include it at compile time. This approach can help manage the binary size and improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/ci_server.yml (1)
Add
cache-dependency-pathto existinggo setupstepsTo ensure consistent caching behavior across all jobs, add
cache-dependency-pathto thego setupsteps in theci-server-lintandci-server-i18njobs.
.github/workflows/ci_server.yml- Jobs:ci-server-lint,ci-server-i18n🔗 Analysis chain
Line range hint
1-56: Consider adding cache-dependency-path to other go setup stepsWhile the addition of
cache-dependency-pathto theci-server-testjob is beneficial, it's worth checking if this optimization can be applied to othergo setupsteps in the workflow for consistency and improved performance across all jobs.Please review the results of the following script to identify other
go setupsteps that might benefit from this optimization:If other
go setupsteps are found without thecache-dependency-pathparameter, consider adding it to those steps as well for consistent caching behavior across all jobs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other jobs in the workflow could benefit from cache-dependency-path # Test: Search for other 'go setup' steps in the workflow rg -A 5 'name: go setup' .github/workflows/ci_server.ymlLength of output: 627
.github/workflows/ci_server.yml
Outdated
| @@ -53,6 +53,7 @@ jobs: | |||
| uses: actions/setup-go@v5 | |||
| with: | |||
| go-version: ${{ env.GO_VERSION }} | |||
| cache-dependency-path: server/go.mod | |||
There was a problem hiding this comment.
💡 Codebase verification
Apply cache-dependency-path to all go setup steps
To ensure consistent and optimized caching across the workflow, add cache-dependency-path: server/go.mod to the remaining go setup steps.
- Jobs to update:
golangci-lintjobcheck forgotten translationsjob
🔗 Analysis chain
Approve the addition of cache-dependency-path
The addition of cache-dependency-path: server/go.mod to the go setup step is a good practice. This parameter helps to optimize the caching of Go module dependencies, potentially speeding up the CI process by reusing cached dependencies when the go.mod file hasn't changed.
To ensure this change is consistently applied across the workflow, let's check if similar jobs could benefit from this optimization:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other jobs in the workflow could benefit from cache-dependency-path
# Test: Search for other 'go setup' steps in the workflow
rg -A 5 'name: go setup' .github/workflows/ci_server.yml
Length of output: 627
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- server/e2e/integration_schema_test.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
server/e2e/integration_schema_test.go (2)
238-273: LGTM - Test for 'number' field type is correctly implementedThe integration test for creating a field of type
numberis appropriately implemented and follows the existing testing patterns.
Line range hint
275-304: LGTM - Test for 'geometryObject' field type is correctly implementedThe integration test for creating a field of type
geometryObjectis appropriately implemented and follows the existing testing patterns.
| //region number | ||
| res = e.POST(endpoint, sid1). | ||
| WithHeader("authorization", "Bearer "+secret). | ||
| WithJSON(map[string]interface{}{ | ||
| "key": "fKey2", | ||
| "type": "number", | ||
| "multiple": false, | ||
| "required": false, | ||
| }). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON(). | ||
| Object() | ||
|
|
||
| res.ContainsKey("id") | ||
|
|
||
| res = e.GET("/api/models/{modelId}", mId1). | ||
| WithHeader("authorization", "Bearer "+secret). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON(). | ||
| Object() | ||
|
|
||
| res.HasValue("id", mId1.String()). | ||
| HasValue("name", "m1"). | ||
| HasValue("description", "m1 desc"). | ||
| HasValue("public", true). | ||
| HasValue("key", ikey1.String()). | ||
| HasValue("projectId", pid). | ||
| HasValue("schemaId", sid1) | ||
|
|
||
| res.Value("schema").Object().Value("fields").Array().Length().IsEqual(5) | ||
| res.Value("createdAt").NotNull() | ||
| res.Value("updatedAt").NotNull() | ||
| res.Value("lastModified").NotNull() | ||
| // endregion |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring repetitive test code into helper functions
The test code for creating fields of different types (number, geometryObject) is repetitive. Consider refactoring the common code into reusable helper functions to improve maintainability and reduce duplication.
Also applies to: 275-304
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
server/e2e/integration_schema_test.go (1)
Line range hint
254-295: Enhance number field type testingSince the PR objective is to add float field support, consider:
- Adding specific test cases for float fields
- Validating number-specific properties (min/max values, precision)
- Testing edge cases (e.g., decimal places, scientific notation)
Add test cases for float-specific scenarios:
WithJSON(map[string]interface{}{ "key": "fKey2", "type": "number", "multiple": false, "required": false, + "min": -1.5, + "max": 1000.55, + "precision": 2 })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- server/e2e/integration_schema_test.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
server/e2e/integration_schema_test.go (2)
Line range hint
210-251: Test structure matches previous patternThe test follows the same pattern as the text field test.
Line range hint
180-295: Refactor common test patterns into helper functionsThe test code for different field types follows similar patterns. As mentioned in the previous review, consider extracting common test patterns into helper functions.
Example refactoring:
func createField(e *httpexpect.Expect, schemaId string, fieldData map[string]interface{}) *httpexpect.Object { return e.POST("/api/schemata/{schemaId}/fields", schemaId). WithHeader("authorization", "Bearer "+secret). WithJSON(fieldData). Expect(). Status(http.StatusOK). JSON(). Object() } func verifyFieldInSchema(e *httpexpect.Expect, modelId string, expectedField map[string]interface{}) { res := e.GET("/api/models/{modelId}", modelId). WithHeader("authorization", "Bearer "+secret). Expect(). Status(http.StatusOK). JSON(). Object() // Verify model metadata res.ContainsSubset(map[string]any{ "name": "m1", "id": modelId, // ... other fields }) // Find and verify the field fields := res.Value("schema").Object().Value("fields").Array() findFieldByKey(fields, expectedField["key"].(string)). ContainsSubset(expectedField) }
| res = e.GET("/api/models/{modelId}", mId1). | ||
| WithHeader("authorization", "Bearer "+secret). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON(). | ||
| Object() | ||
|
|
||
| res.ContainsSubset(map[string]any{ | ||
| "name": "m1", | ||
| "id": mId1.String(), | ||
| "description": "m1 desc", | ||
| "public": true, | ||
| "key": ikey1.String(), | ||
| "projectId": pid, | ||
| "schemaId": sid1, | ||
| }) | ||
|
|
||
| res.Value("createdAt").NotNull() | ||
| res.Value("updatedAt").NotNull() | ||
| res.Value("lastModified").NotNull() | ||
| resf := res.Value("schema").Object().Value("fields").Array() | ||
| resf.Length().IsEqual(3) | ||
| resf.Value(2).Object().ContainsSubset(map[string]any{ | ||
| // "id": "", // generated | ||
| "key": "テスト", | ||
| "type": "text", | ||
| "required": false, | ||
| }) | ||
| // endregion |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making field verification more robust
The current test relies on hardcoded array indices (e.g., resf.Value(2)) to verify fields, which could break if the field order changes. Consider using a helper function to find fields by key instead.
- resf.Value(2).Object().ContainsSubset(map[string]any{
- "key": "テスト",
- "type": "text",
- "required": false,
- })
+ // Add helper function
+ func findFieldByKey(fields *httpexpect.Array, key string) *httpexpect.Object {
+ for i := 0; i < fields.Length().Raw(); i++ {
+ field := fields.Value(i).Object()
+ if field.Value("key").String().Raw() == key {
+ return field
+ }
+ }
+ return nil
+ }
+
+ // Use helper in test
+ findFieldByKey(resf, "テスト").ContainsSubset(map[string]any{
+ "key": "テスト",
+ "type": "text",
+ "required": false,
+ })Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit