feat(server): item fields normalization #1095
Conversation
✅ Deploy Preview for reearth-cms canceled.
|
# Conflicts: # go.work.sum # server/e2e/integration_item_test.go # server/internal/adapter/gql/gqlmodel/convert_item.go # server/internal/adapter/gql/loader_item.go # server/internal/adapter/gql/resolver_mutation_item.go # server/internal/adapter/integration/convert.go # server/internal/adapter/integration/item.go # server/internal/adapter/publicapi/api.go # server/pkg/integrationapi/asset.go # server/pkg/item/item.go # server/pkg/item/reference_test.go # server/pkg/schema/package.go
WalkthroughThis change is a comprehensive refactor of item and asset handling across the server, integration API, and GraphQL adapter layers. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQL
participant IntegrationAPI
participant ConvertContext
participant AssetService
participant ItemService
Client->>GraphQL: Create/Update/Fetch Item
GraphQL->>IntegrationAPI: ToItem/ToVersionedItem (with schema package, context)
IntegrationAPI->>ConvertContext: ResolveAsset/ResolveReferencedItem
ConvertContext->>AssetService: Load assets by ID (if needed)
ConvertContext->>ItemService: Load referenced/meta items by ID (if needed)
IntegrationAPI-->>GraphQL: Item/VersionedItem (with embedded assets/items)
GraphQL-->>Client: Response with item and fields
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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
♻️ Duplicate comments (3)
server/internal/adapter/integration/item.go (3)
145-146: Same asset embedding approach.
The pattern matches lines 39-40. The comment regardingafEmbedusage applies here as well.
152-152: Refactored call toNewVersionedItem.
Identical to line 46 comment.
402-403: Embedding assets on single item retrieval.
Identical pattern to other calls; same observation applies concerningafEmbed.
🧹 Nitpick comments (10)
server/pkg/integrationapi/schema.go (1)
39-42: Passing nil values for the new context-based approach.The function now passes
nilfor the schema package and convert context parameters toNewItemandNewVersionedItem. This is part of the refactoring that centralizes asset and item loading through theConvertContext.Consider adding a comment explaining why nil values are acceptable here, to improve code readability.
- Item: NewItem(i.Item, nil, nil), + Item: NewItem(i.Item, nil, nil), // Schema and ConvertContext not needed for this conversion ReferencedItems: lo.Map(i.ReferencedItems, func(itm *version.Value[*item.Item], _ int) *VersionedItem { - return lo.ToPtr(NewVersionedItem(itm, nil, nil)) + return lo.ToPtr(NewVersionedItem(itm, nil, nil)) // Schema and ConvertContext not needed for this conversionserver/internal/adapter/gql/loader_item.go (1)
91-91: Find all versions
Retrieving all versions is correct. Consider handling large version histories with pagination if the version count grows.server/pkg/item/item.go (1)
51-91: IntroduceNormalizedFieldsfor schema-based expansion
This method constructs a comprehensive field list matching the schema package. Key observations:
- It appends empty fields for schema entries that the item lacks, ensuring completeness.
- Group fields are expanded if they contain valid group IDs. This deeply normalizes nested fields, which is beneficial for a consistent data shape but could be expensive if used frequently on large items.
- The code halts appending more group fields if
Multiple()is false, which is correct.Possible improvement:
- Validate that
GroupSchema(gID)exists (i.e.,groupSchemas[gID]) to avoid nil returns in edge cases.
Otherwise, this is a strong approach to unify item fields with the schema’s structure.if sp == nil { return nil } +if sp.GroupSchema(gID) == nil { + // Optionally handle or log the missing group schema rather than silently ignore +}server/pkg/integrationapi/value.go (2)
52-52: Consider adding missing reverse mapping forValueTypeCheckbox.Currently, the function
NewValueTypecorrectly mapsvalue.TypeCheckboxtoValueTypeCheckbox, but its counterpart functionFromValueTypedoes not handleValueTypeCheckbox. This asymmetry can lead to incomplete conversions.func FromValueType(t *ValueType) value.Type { if t == nil { return "" } switch *t { + case ValueTypeCheckbox: + return value.TypeCheckbox ... }
102-105: EnsureResolveAssethandles all error cases.The code calls
cc.ResolveAsset(aid)without apparent fallback if it returnsnilor encounters an error. Consider adding logic to handle the case where the asset cannot be resolved (e.g., returning a placeholder value or an explicit error).server/internal/adapter/gql/gqlmodel/convert_item.go (1)
33-33: Check for possiblenilreturns fromsp.Schema().If
sp.Schema()is evernil,i.GetTitle(nil)may produce unexpected behavior. Verify that the rest of the system guarantees a non-nil schema.server/pkg/integrationapi/item.go (1)
13-41: Unifying schema usage withConvertContext.
NewVersionedItemeffectively resolves metadata items viacc.ResolveMetaItemand constructs field sets withNewFields. However, line 24 passesnilas theConvertContextwhen creating the schema package for meta fields. This might skip asset or item embedding in metadata fields. Consider passing the sameccor clarifying why anilcontext is acceptable.server/pkg/integrationapi/convert_context.go (2)
30-52: Conditional loading logic looks solid.
The constructor properly initializes and then conditionally loads referenced items, meta items, and assets. However, note thatembedAssetsFilesis passed in yet never stored or used (viaafEmbed).-func NewCC(il item.VersionedList, embedAssets, embedAssetsFiles bool, l Loaders) (*ConvertContext, error) { +func NewCC(il item.VersionedList, embedAssets, embedAssetsFiles bool, l Loaders) (*ConvertContext, error) { cc := &ConvertContext{ aFiles: make(map[asset.ID]*asset.File), aBaseURL: l.AssetURL, aEmbed: embedAssets, - // Possibly missing usage for embedAssetsFiles in the below fields loaders: l, } ... }
68-88: Reference item loading logic.
The approach for finding all reference-type fields and loading the corresponding items is clear. Consider applying deduplication (as with assets) if needed to reduce repeated lookups, but it might be fine if the loader itself handles it.server/internal/adapter/integration/item.go (1)
454-457: Clear logic inshouldEmbedAsset.
This function adequately distinguishes normal vs file-level embedding. However, the second return value is unused down the line.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
server/e2e/integration_item_project_test.go(2 hunks)server/e2e/integration_item_test.go(11 hunks)server/internal/adapter/gql/gqlmodel/convert_item.go(3 hunks)server/internal/adapter/gql/gqlmodel/convert_item_test.go(5 hunks)server/internal/adapter/gql/loader_item.go(5 hunks)server/internal/adapter/gql/resolver_item.go(5 hunks)server/internal/adapter/integration/convert.go(2 hunks)server/internal/adapter/integration/item.go(6 hunks)server/internal/adapter/integration/schema.go(4 hunks)server/internal/adapter/publicapi/api.go(3 hunks)server/internal/infrastructure/aws/webhook.go(1 hunks)server/internal/infrastructure/gcp/webhook.go(1 hunks)server/pkg/integrationapi/asset.go(3 hunks)server/pkg/integrationapi/asset_test.go(5 hunks)server/pkg/integrationapi/convert.go(1 hunks)server/pkg/integrationapi/convert_context.go(1 hunks)server/pkg/integrationapi/event.go(1 hunks)server/pkg/integrationapi/event_test.go(2 hunks)server/pkg/integrationapi/item.go(1 hunks)server/pkg/integrationapi/schema.go(1 hunks)server/pkg/integrationapi/value.go(2 hunks)server/pkg/integrationapi/value_test.go(1 hunks)server/pkg/item/item.go(3 hunks)server/pkg/item/item_test.go(2 hunks)server/pkg/item/reference.go(0 hunks)server/pkg/item/reference_test.go(0 hunks)server/pkg/schema/package.go(1 hunks)
💤 Files with no reviewable changes (2)
- server/pkg/item/reference_test.go
- server/pkg/item/reference.go
🧰 Additional context used
🧬 Code Graph Analysis (11)
server/pkg/integrationapi/value_test.go (1)
server/pkg/integrationapi/value.go (1)
NewValueType(52-91)
server/pkg/integrationapi/asset_test.go (1)
server/pkg/integrationapi/asset.go (3)
NewAssetFile(55-81)NewAssetArchiveExtractionStatus(35-53)NewPreviewType(83-107)
server/internal/infrastructure/aws/webhook.go (1)
server/pkg/integrationapi/event.go (2)
NewEventWith(41-67)Event(11-18)
server/pkg/integrationapi/schema.go (2)
server/pkg/item/item.go (2)
ItemModelSchema(280-287)Item(16-31)server/pkg/integrationapi/item.go (2)
NewItem(73-84)NewVersionedItem(13-41)
server/internal/adapter/integration/schema.go (1)
server/pkg/integrationapi/value.go (1)
NewValueType(52-91)
server/internal/adapter/gql/gqlmodel/convert_item_test.go (2)
server/pkg/schema/package.go (1)
NewPackage(15-22)server/internal/adapter/gql/gqlmodel/convert_item.go (2)
ToItem(12-36)ToVersionedItem(51-68)
server/internal/adapter/gql/resolver_item.go (4)
server/internal/usecase/interactor/schema.go (1)
Schema(17-20)server/internal/usecase/interfaces/item.go (1)
Item(127-151)server/internal/adapter/gql/gqlmodel/convert_item.go (1)
ToItem(12-36)server/internal/usecase/interfaces/model.go (1)
Model(62-78)
server/pkg/item/item.go (10)
server/pkg/item/fields.go (1)
Fields(10-10)server/pkg/item/id.go (4)
ThreadID(15-15)UserID(16-16)IntegrationID(17-17)ID(8-8)server/pkg/schema/package.go (1)
Package(8-13)server/pkg/schema/schema.go (1)
Schema(15-21)server/pkg/item/field.go (2)
Field(9-13)NewField(15-24)server/pkg/value/multiple.go (2)
NewMultiple(13-25)Multiple(8-11)server/pkg/value/type.go (1)
Type(3-3)server/pkg/value/group.go (1)
TypeGroup(8-8)server/pkg/schema/type_property.go (1)
TypePropertyMatch(34-53)server/pkg/schema/field_group.go (1)
FieldGroup(7-9)
server/pkg/integrationapi/convert_context.go (7)
server/pkg/asset/map.go (1)
Map(5-5)server/pkg/item/list.go (1)
VersionedList(34-34)server/pkg/id/id.go (1)
ItemID(178-178)server/pkg/item/item.go (1)
Versioned(33-33)server/pkg/item/fields.go (1)
Fields(10-10)server/pkg/value/reference.go (1)
TypeReference(8-8)server/pkg/integrationapi/asset.go (1)
NewAsset(8-33)
server/e2e/integration_item_test.go (1)
server/pkg/id/key.go (1)
NewKey(26-29)
server/pkg/integrationapi/event.go (2)
server/pkg/asset/asset.go (1)
URLResolver(26-26)server/pkg/integrationapi/convert.go (1)
New(17-45)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-server / test
- GitHub Check: ci-server / lint
🔇 Additional comments (82)
server/pkg/integrationapi/value_test.go (1)
219-219: Clean function rename alignment.The change from
ToValueTypetoNewValueTypealigns with the broader refactoring in the integration API where conversion functions were renamed fromTo*toNew*prefixes for more consistent naming conventions.server/internal/infrastructure/aws/webhook.go (1)
24-24: Updated function call matches new signature.The empty string parameter has been correctly removed from the
NewEventWithcall to match the updated function signature. This change properly aligns with the refactored API where this parameter is no longer needed.server/pkg/item/item_test.go (2)
86-86: Type change from[]*FieldtoFields.The test assertion has been updated to use the new
Fieldstype instead of a raw slice of pointers toField, which aligns with the refactoring of theItemstruct's field type.
101-101: Type change from[]*FieldtoFields.The test assertion has been updated to use the new
Fieldstype instead of a raw slice of pointers toField, which aligns with the refactoring of theItemstruct's field type.server/internal/adapter/integration/convert.go (2)
125-126: Key field assignment commented out.The Key field assignment is intentionally commented out as part of the field normalization refactoring. This aligns with the introduction of the
ConvertContextand normalized field representations where explicit Key setting may no longer be necessary.Verify that the downstream code that consumes these
ItemFieldParamobjects doesn't rely on the Key field being set at this point, and that it's either not needed or set elsewhere in the refactored code path.
154-155: Key field assignment commented out.The Key field assignment is intentionally commented out as part of the field normalization refactoring. This aligns with the introduction of the
ConvertContextand normalized field representations where explicit Key setting may no longer be necessary.Verify that the downstream code that consumes these
ItemFieldParamobjects doesn't rely on the Key field being set at this point, and that it's either not needed or set elsewhere in the refactored code path.server/internal/adapter/integration/schema.go (1)
115-115: Function calls renamed fromToValueTypetoNewValueTypeThese changes are part of a broader refactoring effort to rename conversion functions from
To*toNew*across the integration API. The function behavior remains the same, but names are now more consistent.Also applies to: 176-176, 227-227, 294-294
server/internal/infrastructure/gcp/webhook.go (1)
24-24: Simplified function signature forNewEventWithThe empty string parameter has been removed from the
NewEventWithcall, reflecting the updated function signature in the integration API that no longer needs this parameter. This change simplifies the API surface while preserving functionality.server/pkg/integrationapi/asset_test.go (3)
20-20: Function calls renamed fromToAssetFiletoNewAssetFileThese changes align the test code with the renaming of the asset conversion functions in the codebase. The
To*prefix has been standardized toNew*prefix while maintaining the same function behavior.Also applies to: 45-45, 54-54
160-160: Function call renamed fromToAssetArchiveExtractionStatustoNewAssetArchiveExtractionStatusThis change aligns the test code with the renaming pattern of asset conversion functions from
To*toNew*in the integration API package.
227-227: Function call renamed fromToPreviewTypetoNewPreviewTypeThis change is part of the consistent renaming pattern from
To*toNew*for conversion functions in the integration API package.server/internal/adapter/gql/gqlmodel/convert_item_test.go (2)
30-30: Updated test to useschema.PackageabstractionThe test now creates a schema package and passes it to
ToItem, aligning with the refactored function signature that uses a single*schema.Packageparameter instead of separate schema objects. This approach centralizes schema handling and simplifies the API.Also applies to: 82-82
134-134: UpdatedToVersionedItemtest to useschema.PackageSimilar to the
ToItemtest changes, these updates adapt the tests to use the newschema.Packageabstraction, which encapsulates all schema-related information in a single container. This provides a more unified approach to schema handling across the GraphQL adapter layer.Also applies to: 151-151, 160-160
server/pkg/integrationapi/event_test.go (2)
83-84: Parameter removal aligns with refactored method signatures.The removal of the version string parameter (
v) from theNewfunction call is consistent with the broader refactoring of the integration API conversion logic.
163-163: Test updated to match new function signature.The call to
NewEventWithnow omits the version string parameter, correctly reflecting the updated method signature that no longer requires this parameter.server/pkg/integrationapi/schema.go (1)
37-37: Function signature simplified by removing the assets parameter.The
NewItemModelSchemafunction has been simplified by removing theassetsparameter, which is consistent with the introduction of the newConvertContextapproach that centralizes asset and item resolution.server/e2e/integration_item_project_test.go (2)
157-168: Test expectations updated to reflect new field structure.The test has been correctly updated to:
- Use
map[string]anyinstead ofmap[string]stringto accommodate more flexible value types- Include a new asset field with a nil value, reflecting the updated item field normalization
This ensures the test validates the behavior after the field normalization changes.
189-200: Consistent update to second assertion.The changes to this assertion match the pattern used in the first one, maintaining consistency in how asset fields are represented.
server/internal/adapter/gql/resolver_item.go (4)
161-164: Simplified schema retrieval using FindByModel.This change improves the code by fetching a single schema package by model ID instead of multiple schemas and group schemas. This aligns with the broader refactoring to unify schema handling under the
schema.Packageabstraction.
176-177: Simplified item conversion with schema package.The call to
ToItemnow uses the schema package directly, which streamlines the conversion by avoiding the need to pass multiple schema-related parameters.
207-210: Consistent schema retrieval approach.This change follows the same pattern as in
CreateItem, maintaining consistency in how schemas are retrieved across resolver methods.
213-214: Simplified item conversion with schema package.Same improvement as in the
CreateItemresolver, maintaining consistency.server/pkg/integrationapi/convert.go (4)
17-17: Function signature improvement: Removed unused parameterThe function signature has been simplified by removing the unused
v stringparameter, as indicated by the comment on line 18. This improves code clarity.
21-21: Updated function call to match new signatureThis call correctly adapts to the updated signature of
NewEventWiththat no longer takes a string parameter.
25-25: Function renaming for consistencyThe function has been renamed from
ToAssetFiletoNewAssetFileto follow a more consistent naming convention for factory methods (New*instead ofTo*).
29-29: Simplified function callsThe function calls to
NewVersionedItemandNewItemModelSchemahave been simplified by removing unnecessary parameters, which improves readability.Also applies to: 31-31
server/internal/adapter/publicapi/api.go (2)
36-37: Improved handler naming conventionThe handler functions have been renamed from
PublicApi*to more concise names that follow idiomatic Go naming conventions. This improves code readability and maintainability.
40-40: Consistent function namingFunctions have been renamed from
PublicApi*to follow lowercase naming conventions (itemOrAssetHandler,itemListHandler,assetHandler). This is more aligned with Go's idiomatic style where public API prefixes aren't needed in internal function names.Also applies to: 64-64, 115-115
server/pkg/schema/package.go (1)
79-81: Refactored to reduce code duplicationThe
Fieldmethod now delegates to the more flexibleFieldByIDOrKeymethod, which consolidates field lookup logic. This improves maintainability and reduces code duplication.This refactoring aligns with the broader changes to unify schema handling under the
schema.Packageabstraction, making the code more consistent across the codebase.server/pkg/integrationapi/event.go (2)
41-41: Simplified function signatureThe
NewEventWithfunction's signature has been simplified by removing the unusedv stringparameter, making the API cleaner.
46-46: Updated call to reflect parameter removalThe call to
Newhas been updated to match its new signature without thevparameter. This maintains consistency with the changes inconvert.go.server/internal/adapter/gql/loader_item.go (11)
13-13: Importedlogfrom reearthx
No immediate concerns. Ensure logging is appropriately leveled and does not cause excessive overhead.
41-41: Retrieve items by their IDs
Fetching items usingFindByIDsis straightforward. Check whether bulk-fetching large ID lists can handle concurrency or pagination if the dataset grows significantly.
46-48: Graceful exit for empty result
This check avoids downstream nil pointer issues. Good defensive programming.
50-50: Single schema retrieval
Fetching a singlespbased on the first item's model aligns with the new consolidated schema approach.
55-56: Mapping items to GraphQL
Passing a singleschema.Packagetogqlmodel.ToItemis consistent with the updated unification strategy.
75-81: Versioned item schema retrieval
Analogous logic to the “Fetch” method for versioned items. This maintains consistency. No major issues noted.
96-98: Return empty if versions are missing
Same robust pattern for nil checks as in “Fetch.” This prevents runtime errors.
100-100: Schema retrieval
Similar single-model schema fetch ensures consistent logic across the codebase.
105-107: Mapping versioned items
Usinglo.Mapagain to build GraphQL items from versioned data. Seamless and clear.
131-131: Item conversion in Search
itm := gqlmodel.ToItem(i, sp)aligns with the single package approach. Good consistency with the rest of the file.
18-18:❓ Verification inconclusive
Imported
tracefrom opencensus
Adding distributed tracing is beneficial for monitoring. Make sure spans are properly ended in all error paths, so resources aren’t leaked.Would you like me to scan the repo for any unended spans or partial usage of trace to confirm correctness?
🏁 Script executed:
#!/bin/bash ast-grep --pattern $'trace.StartSpan($_, $_)'Length of output: 257
Verify Proper Ending of Tracing Spans
The AST grep confirmed that distributed tracing is initiated via
trace.StartSpanin both the GraphQL loader (server/internal/adapter/gql/loader_item.go:111) and the Mongo search (server/internal/infrastructure/mongo/item_search.go:25). Please ensure that for every span started in these locations, the correspondingspan.End()is called on all execution paths—including error/exception cases—to prevent resource leaks.server/pkg/item/item.go (2)
21-28: Reordered and introduced fields in Item struct
The new or updated fields (fields Fields,integration *IntegrationID,isMetadata) are consistent with the revised architecture. Ensure null pointer usage is safe, especially for optional integration references.
121-121: Delegating field lookups toi.fields.Field()
This direct delegation is clean and avoids redundant loops. No issues noted.server/e2e/integration_item_test.go (8)
88-88: Use of fixed key"text"instead of random
Replacingid.RandomKey()withid.NewKey("text")helps produce stable results and simplifie test expectations.
1528-1539: Added asset field with nil value in test
These lines introduce an additional map for “asset” references set tonil. This tests default or empty states for asset fields. Good thoroughness in coverage.
1568-1580: Metadata field addition in create item tests
Here you’re adding “metadataFields” referencing a boolean field. This ensures coverage of sets that mix normal and metadata fields. Tests appear correct and well-structured.
1730-1735: Update item’s asset field
Switching the item’s second field to reference an existing asset ID. This confirms correct patch behavior and referencing.
1833-1838: Restoring group field
Ensuring that the group field forfId6references multiple group IDs. This validates the item’s ability to handle multiple references in arrays.
1881-1886: Extending group references
Adding a third group reference tests “multiple” group fields. This thorough coverage is beneficial for verifying the system’s expansions.
2070-2074: Group field in retrieved item
On retrieval, verifying we get the correct group field ID, type, and values array. All changes align with group item expansions.
2153-2181: Assert item reflection
- Checking
parentsis empty andrefsis["latest"]confirms correct references for newly created items.- Asserting exactly two fields: one text, one asset.
- Conditional check for embedded asset object vs. plain asset ID clarifies coverage of both embedded and non-embedded asset responses.
All changes are consistent with the broader refactor.
server/pkg/integrationapi/value.go (1)
93-99: Check for potentialnilvalues when callingv.First().Here, if
vis empty,v.First()returnsnil. CallingNewValue(nil, sf, cc)can result in a panic unless you handle anilargument insideNewValue.server/internal/adapter/gql/gqlmodel/convert_item.go (4)
12-12: New function signature looks good.Accepting a
schema.Packageinstead of separate schema parameters simplifies handling of group vs. main schema. This is consistent with the unified approach across the codebase.
31-31: No direct issues with assigningtoItemFields.However, verify that
toItemFieldscorrectly handles all field scenarios, including group fields. The logic looks sound but consider adding tests for edge cases (e.g., emptyschema.Package).
37-37: New helper function approach is clear.
toItemFieldsconsolidates the logic for normalizing fields in one place. This is a clean design choice.
39-40: Ensuresp.Field(f.FieldID())is valid.If
sp.Field(f.FieldID())returnsnil, calling.Multiple()on it can lead to a panic. Confirm thatNormalizedFieldsreferences only valid field IDs in the schema.server/pkg/integrationapi/asset.go (7)
8-8: Parameter rename toembedis clearer.Renaming from a more ambiguous
alltoembedclarifies the intent of controlling recursive embedding. This improves readability.
26-31: Synchronize preview and file creation logic.These lines correctly invoke
NewPreviewType,NewAssetFile, andNewAssetArchiveExtractionStatus. Just ensure errors in subcalls (e.g., a missing file) won't produce partially populated assets.
35-35: Straightforward approach for archive status mapping.
NewAssetArchiveExtractionStatusreturnsnilon unknown status, which is consistent with the existing pattern.
42-48: Enum mappings confirmed.The switch covers known statuses (
Done,Failed,InProgress,Pending) and defaults tonil. This is a clear fallback mechanism.
55-55: Good separation of concerns inNewAssetFile.The function is well-defined for building file metadata. Passing
embedkeeps the logic consistent withNewAsset.
61-63: Watch for potential infinite recursion in nested files.If a parent asset file references itself as a child, recursively calling
NewAssetFile(c, true)might lead to an infinite loop. Confirm that cyclical references are disallowed or handled.
83-83: Simple rename toNewPreviewTypeis correct.The function maintains the previous mappings while clarifying naming consistency.
server/pkg/integrationapi/item.go (2)
43-71: Prevent potential duplication or cyclic expansions innewReferencedItems.This method builds additional referenced items by scanning fields for
value.TypeReference. Verify that cyclical references, referenced item duplicates, or deep dependency trees won't cause performance or recursion issues.
73-98: Ensure schema field lookups are always valid.
NewFieldscallssf := sp.Field(f.FieldID()); if the schema package lacks that field, subsequent operations likesf.Key()can panic. Double-check thati.NormalizedFields(sp)doesn’t produce invalid field IDs.server/pkg/integrationapi/convert_context.go (5)
16-17: UnusedafEmbedfield.
TheafEmbedfield is declared but never utilized. If it's intended for embedding files, please implement the logic; otherwise, remove to avoid confusion.
54-66: Efficient asset loading.
The use oflo.Uniq(lo.FlatMap(...))to gather and deduplicate asset IDs is a neat approach. Ensure that potential errors fromc.loaders.Assetare handled properly, but overall this looks good.
90-102: Meta items loading strategy.
This logic parallels the reference loading. It seems well-structured and consistent with the rest of the constructor flow.
128-133: Meta item resolution is straightforward.
This method is direct and consistent with other resolution functions. It properly handles a nil context and empty list.
135-140: Referenced item resolution is straightforward.
The function closely mirrorsResolveMetaItem. The design is coherent and maintains consistency.server/internal/adapter/integration/item.go (11)
8-8: Import addition.
No concerns here: importing"github.com/reearth/reearth-cms/server/internal/usecase"is consistent with existing usage.
14-14: Model package import.
The addition of"github.com/reearth/reearth-cms/server/pkg/model"is valid for referencing themodel.Modeltype.
39-40: Embedding assets integration.
InvokingshouldEmbedAssetand using both return values is correct, but note thatafEmbedmight not be leveraged in theConvertContext. Verify the intended usage.
46-46: Refactored call toNewVersionedItem.
Passing the newConvertContexttoNewVersionedItemaligns properly with the updated signature.
137-137: Query updated to find items by schema.
Switching toFindBySchemais consistent with the rest of the refactor. Ensure that any explicit query filters are no longer needed or are handled elsewhere.
360-361: Embedding assets for item update.
UsingshouldEmbedAssetis consistent. The second bool might remain unused in the current code.
366-366: Return of the updated item with embedding.
Passingccto create the updated versioned item is correct.
408-408: Returning item with new context.
Consistent usage ofNewVersionedItemfor a single item fetch response.
446-449: Explicit asset embedding flags.
NewCC(item.VersionedList{i}, true, false, loaders(ctx))setstrueforaEmbedandfalseforafEmbed; verify that ignoring file embedding is intentional.
451-451: Pointer conversion for consistent return.
Useslo.ToPtr(integrationapi.NewVersionedItem(...)), aligning with other item creation flows.
462-470: Loader functions.
These lambdas provide a clean integration layer. The pattern for retrieving assets and items by IDs is straightforward, with separate functions for each.
| sp, err := usecases(ctx).Schema.FindByModel(ctx, itm[0].Value().Model(), op) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &gqlmodel.PublishItemPayload{ | ||
| Items: lo.Map(itm, func(t item.Versioned, _ int) *gqlmodel.Item { return gqlmodel.ToItem(t, s, nil) }), | ||
| Items: lo.Map(itm, func(t item.Versioned, _ int) *gqlmodel.Item { return gqlmodel.ToItem(t, sp) }), | ||
| }, nil |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Schema retrieval and array safety verification needed.
The code now uses the model ID from the first item in the result array.
While this approach is more efficient for retrieving schemas, there's a potential issue if the itm array is empty. Consider adding a check to handle this case.
🏁 Script executed:
#!/bin/bash
# Description: Check for error handling when item array might be empty
# Search for other places where the first element of an array is accessed without a check
rg -A 5 -B 5 '\[[0-9]\]' --include "*.go" | grep -v "test"Length of output: 500
Schema Retrieval and Array Safety Check Required
The current implementation retrieves the schema using the model ID from the first item in the itm slice, optimizing the retrieval process. However, if itm is empty, directly accessing itm[0] can result in a runtime error. Please add a guard clause to verify that itm contains at least one element before proceeding. For example:
if len(itm) == 0 {
return nil, errors.New("no items found")
}
sp, err := usecases(ctx).Schema.FindByModel(ctx, itm[0].Value().Model(), op)
if err != nil {
return nil, err
}
return &gqlmodel.PublishItemPayload{
Items: lo.Map(itm, func(t item.Versioned, _ int) *gqlmodel.Item { return gqlmodel.ToItem(t, sp) }),
}, nilAffected Location
- File:
server/internal/adapter/gql/resolver_item.go, Lines: 243-250
| sp, err := usecases(ctx).Schema.FindByModel(ctx, res[0].Value().Model(), op) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &gqlmodel.UnpublishItemPayload{ | ||
| Items: lo.Map(res, func(t item.Versioned, _ int) *gqlmodel.Item { return gqlmodel.ToItem(t, s, nil) }), | ||
| Items: lo.Map(res, func(t item.Versioned, _ int) *gqlmodel.Item { return gqlmodel.ToItem(t, sp) }), | ||
| }, nil |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Schema retrieval and array safety verification needed.
Similar to the PublishItem resolver, this code accesses the first element of the res array without checking if it's empty.
Consider adding a check to ensure the array isn't empty before accessing its elements.
🏁 Script executed:
#!/bin/bash
# Description: Check error handling in unpublish case
# Search for how errors are handled in similar array-returning functions
rg -A 10 'Unpublish\(' --include "*.go" | grep -v "test"Length of output: 498
Ensure Array Safety for res[0] in Unpublish Resolver
In server/internal/adapter/gql/resolver_item.go (lines 264-270), the code directly accesses res[0] without confirming that the slice is non-empty. Similar to the check present in the PublishItem resolver, please add a guard (e.g., verifying that len(res) > 0) before accessing the first element. This will prevent potential index-out-of-bound errors when res is empty.
| func (c *ConvertContext) ResolveAsset(id asset.ID) *Asset { | ||
| if c == nil || c.aMap == nil { | ||
| return nil | ||
| } | ||
|
|
||
| a, ok := c.aMap[id] | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| url := "" | ||
| if c.aBaseURL != nil { | ||
| url = c.aBaseURL(a) | ||
| } | ||
|
|
||
| f := c.aFiles[id] | ||
|
|
||
| return NewAsset(a, f, url, c.aEmbed) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential mismatch with afEmbed.
ResolveAsset constructs the asset but exclusively checks c.aEmbed. If there's a reason to distinguish file embedding separately (afEmbed), it should be passed here. Otherwise, consider removing afEmbed.
Summary by CodeRabbit