Uh oh!
There was an error while loading. Please reload this page.
enable using a compiled model - #39
Conversation
…an index to EntityId on the snapshot table.
WalkthroughThe changes refactor JSON serialization and deserialization logic across three entity configuration classes. Serialization and deserialization operations are now delegated to new or updated static methods, each accepting optional Changes
Sequence Diagram(s)sequenceDiagram
participant EF as Entity Framework
participant Config as *EntityConfig (Change, Commit, or Snapshot)
participant Serializer as JsonSerializer
EF->>Config: Configure(entity, builder)
Config->>Config: Check if EF.IsDesignTime
alt Design-time
Config->>Serializer: Serialize/Deserialize with null options
else Runtime
Config->>Serializer: Serialize/Deserialize with provided options
end
Poem
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 ( |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SIL.Harmony/Db/EntityConfig/ChangeEntityConfig.cs(1 hunks)src/SIL.Harmony/Db/EntityConfig/CommitEntityConfig.cs(2 hunks)src/SIL.Harmony/Db/EntityConfig/SnapshotEntityConfig.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/SIL.Harmony/Db/EntityConfig/CommitEntityConfig.cs (1)
src/SIL.Harmony/Db/EntityConfig/SnapshotEntityConfig.cs (1)
Serialize(44-47)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (10)
src/SIL.Harmony/Db/EntityConfig/CommitEntityConfig.cs (3)
29-31: Added static serialization methods for JSON conversionThe change extracts JSON serialization and deserialization logic into dedicated static methods, which aligns with the approach used in other entity configurations and supports compiled model usage.
37-41: Verify behavior of null handling between conversion and methodThere's a discrepancy between the handling of null values:
- The converter falls back to
new()when deserialization returns null- The
Deserializemethod throws an exception when deserialization returns nullMake sure this is intentional, as it could lead to inconsistent behavior depending on how the method is called.
43-46: Method implementation looks goodThe static serialization method is correctly implemented, consistent with the pattern established in other entity configurations.
src/SIL.Harmony/Db/EntityConfig/ChangeEntityConfig.cs (4)
9-14: Good pattern for constructor optionsThe constructor pattern with both a parameterized version and a parameterless version defaulting to
JsonSerializerOptions.Defaultis a good approach to provide flexibility while maintaining backward compatibility.
20-37: Well-implemented conditional serialization based on design timeThe conditional property configuration based on
EF.IsDesignTimeis a good implementation that properly addresses the requirements for supporting compiled models. The approach of holding the property in a variable first and then configuring it conditionally improves code readability.
40-44: Properly implemented method with error handlingThe public static method correctly implements deserialization with error handling when JSON conversion fails, making it suitable for both internal and external use.
46-49: Well-structured serialization methodThe method is properly implemented, with the appropriate public static modifier to support compiled model requirements.
src/SIL.Harmony/Db/EntityConfig/SnapshotEntityConfig.cs (3)
20-35: Good implementation of conditional serializationThe conditional property configuration based on
EF.IsDesignTimeis well-structured and consistent with the implementation in ChangeEntityConfig. The approach properly addresses the requirements for supporting compiled models.
38-42: Appropriate method implementation with error handlingThe public static method correctly implements deserialization with appropriate error handling, making it suitable for both internal and external use.
44-47: Well-structured serialization methodThe serialization method is properly implemented, with appropriate public static modifiers to support compiled model requirements.
| m => JsonSerializer.Serialize(m, (JsonSerializerOptions?)null), | ||
| json => JsonSerializer.Deserialize<CommitMetadata>(json, (JsonSerializerOptions?)null) ?? new() | ||
| m => Serialize(m, null), | ||
| json => Deserialize(json, null) ?? new() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding EF.IsDesignTime conditional logic to match other config classes
Other entity configurations (ChangeEntityConfig and SnapshotEntityConfig) conditionally apply serialization options based on whether EF is in design-time mode. This file should follow the same pattern for consistency.
- m => Serialize(m, null),- json => Deserialize(json, null) ?? new()+ var metadataProperty = builder.Property(c => c.Metadata)+ .HasColumnType("jsonb");+ if (EF.IsDesignTime)+ {+ metadataProperty.HasConversion(+ m => Serialize(m, null),+ json => Deserialize(json, null) ?? new()+ );+ }+ else+ {+ metadataProperty.HasConversion(+ m => Serialize(m, jsonSerializerOptions),+ json => Deserialize(json, jsonSerializerOptions) ?? new()+ );+ }Committable suggestion skipped: line range outside the PR's diff.
compiled models require static access to all converters. In order for this to work it requires some customization of the compiled model, but this does seem to be a valid workaround for issues using a compiled model. See sillsdev/languageforge-lexbox#1609 for the workaround
Summary by CodeRabbit