-
Notifications
You must be signed in to change notification settings - Fork 0
Serializer Improvements #149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThese changes introduce attribute-based property name mapping for JSON serialization and deserialization. New attributes (MapName, MapInputName, MapOutputName) enable specifying how property names map to JSON field names. An IPropertyMapper interface is introduced with implementations for common transformations (CamelCase, PascalCase, SnakeCase, ProvidedName). JsonSerializer is enhanced to detect and apply these attributes during object serialization. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JsonSerializer
participant AttributeReader
participant Mapper
participant ObjectBuilder
Client->>JsonSerializer: serialize(object)
JsonSerializer->>AttributeReader: hasMapNameAttributes(object)
alt Has Map Attributes
AttributeReader-->>JsonSerializer: true
loop For each property
JsonSerializer->>AttributeReader: getAttributeForProperty(property)
AttributeReader-->>JsonSerializer: Attribute(mapper/string)
JsonSerializer->>Mapper: map(propertyName)
Mapper-->>JsonSerializer: mappedName
JsonSerializer->>JsonSerializer: includePropertyAs(mappedName)
end
JsonSerializer-->>Client: JSON string
else No Map Attributes
AttributeReader-->>JsonSerializer: false
JsonSerializer->>JsonSerializer: delegateToParentSerializer()
JsonSerializer-->>Client: JSON string
end
sequenceDiagram
participant Client
participant JsonSerializer
participant AttributeReader
participant MapperFactory
participant Constructor
Client->>JsonSerializer: deserialize(class, jsonString)
JsonSerializer->>AttributeReader: hasMapNameAttributes(class)
alt Has Map Attributes (Input)
AttributeReader-->>JsonSerializer: true
JsonSerializer->>JsonSerializer: parseJSON(jsonString)
loop For each JSON field
JsonSerializer->>AttributeReader: getInputMapping(jsonField)
alt Mapper
AttributeReader->>MapperFactory: getMapperForField()
MapperFactory-->>JsonSerializer: IPropertyMapper
JsonSerializer->>MapperFactory: reverseMap(jsonField)
else String Mapping
AttributeReader-->>JsonSerializer: propertyName
end
JsonSerializer->>JsonSerializer: mapToProperty(propertyName)
end
JsonSerializer->>Constructor: new class(mappedProperties)
Constructor-->>JsonSerializer: instance
JsonSerializer-->>Client: object
else No Map Attributes
AttributeReader-->>JsonSerializer: false
JsonSerializer->>JsonSerializer: delegateToParentDeserializer()
JsonSerializer-->>Client: object
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. 🔧 PHPStan (2.1.31)Invalid configuration: Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Objects/Serializers/Attributes/MapName.php (1)
12-15: Consider removing redundant PHPDoc @Property tags.The
@propertytags are typically for magic properties accessed via__get/__set. Since these are constructor-promoted public properties with explicit type declarations in the signature (lines 20-21), the PHPDoc is redundant.Apply this diff to remove the redundant PHPDoc:
-/** - * @property string|IPropertyMapper $input Mapper used for deserialization operations - * @property string|IPropertyMapper $output Mapper used for serialization operations. Defaults to input. - */ #[Attribute(Attribute::TARGET_PROPERTY | Attribute::TARGET_CLASS)] class MapNamesrc/Objects/Serializers/JsonSerializer.php (1)
153-166: Validate mapper class implements IPropertyMapper.When a mapper class exists but doesn't implement
IPropertyMapper(line 158), the code silently falls through and returns the class name as a string. This could lead to confusing behavior.Apply this diff to add validation:
if (is_string($mapper)) { if (class_exists($mapper)) { $mapperInstance = new $mapper(); if ($mapperInstance instanceof IPropertyMapper) { return $mapperInstance->map($propertyName); + } else { + throw new RuntimeException( + "Mapper class '{$mapper}' must implement IPropertyMapper" + ); } } return $mapper; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Objects/Serializers/Attributes/MapInputName.php(1 hunks)src/Objects/Serializers/Attributes/MapName.php(1 hunks)src/Objects/Serializers/Attributes/MapOutputName.php(1 hunks)src/Objects/Serializers/JsonSerializer.php(2 hunks)src/Objects/Serializers/Mappers/CamelCaseMapper.php(1 hunks)src/Objects/Serializers/Mappers/IPropertyMapper.php(1 hunks)src/Objects/Serializers/Mappers/PascalCaseMapper.php(1 hunks)src/Objects/Serializers/Mappers/ProvidedNameMapper.php(1 hunks)src/Objects/Serializers/Mappers/SnakeCaseMapper.php(1 hunks)tests/Unit/SerializerTests/JsonSerializerTest.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
src/Objects/Serializers/Attributes/MapInputName.php (4)
src/Objects/Serializers/Attributes/MapName.php (2)
Attribute(16-29)__construct(19-28)src/Objects/Serializers/Attributes/MapOutputName.php (2)
Attribute(12-18)__construct(15-17)src/Objects/Serializers/Mappers/ProvidedNameMapper.php (1)
__construct(11-11)tests/Unit/SerializerTests/JsonSerializerTest.php (6)
__construct(19-23)__construct(28-34)__construct(39-44)__construct(49-53)__construct(58-64)__construct(70-74)
src/Objects/Serializers/Mappers/CamelCaseMapper.php (5)
src/Objects/Support/Types/Str.php (1)
Str(7-56)src/Objects/Serializers/Mappers/IPropertyMapper.php (1)
map(11-11)src/Objects/Serializers/Mappers/PascalCaseMapper.php (1)
map(13-16)src/Objects/Serializers/Mappers/ProvidedNameMapper.php (1)
map(13-16)src/Objects/Serializers/Mappers/SnakeCaseMapper.php (1)
map(13-16)
src/Objects/Serializers/Mappers/ProvidedNameMapper.php (5)
src/Objects/Serializers/Attributes/MapInputName.php (1)
__construct(15-17)src/Objects/Serializers/Attributes/MapName.php (1)
__construct(19-28)src/Objects/Serializers/Attributes/MapOutputName.php (1)
__construct(15-17)tests/Unit/SerializerTests/JsonSerializerTest.php (6)
__construct(19-23)__construct(28-34)__construct(39-44)__construct(49-53)__construct(58-64)__construct(70-74)src/Objects/Serializers/Mappers/IPropertyMapper.php (1)
map(11-11)
src/Objects/Serializers/Mappers/SnakeCaseMapper.php (5)
src/Objects/Support/Types/Str.php (1)
Str(7-56)src/Objects/Serializers/Mappers/CamelCaseMapper.php (1)
map(13-16)src/Objects/Serializers/Mappers/IPropertyMapper.php (1)
map(11-11)src/Objects/Serializers/Mappers/PascalCaseMapper.php (1)
map(13-16)src/Objects/Serializers/Mappers/ProvidedNameMapper.php (1)
map(13-16)
src/Objects/Serializers/Mappers/PascalCaseMapper.php (5)
src/Objects/Support/Types/Str.php (1)
Str(7-56)src/Objects/Serializers/Mappers/CamelCaseMapper.php (1)
map(13-16)src/Objects/Serializers/Mappers/IPropertyMapper.php (1)
map(11-11)src/Objects/Serializers/Mappers/ProvidedNameMapper.php (1)
map(13-16)src/Objects/Serializers/Mappers/SnakeCaseMapper.php (1)
map(13-16)
src/Objects/Serializers/Attributes/MapName.php (4)
src/Objects/Serializers/Attributes/MapInputName.php (2)
Attribute(12-18)__construct(15-17)src/Objects/Serializers/Attributes/MapOutputName.php (2)
Attribute(12-18)__construct(15-17)tests/Unit/SerializerTests/JsonSerializerTest.php (7)
MapName(67-75)__construct(19-23)__construct(28-34)__construct(39-44)__construct(49-53)__construct(58-64)__construct(70-74)src/Objects/Serializers/Mappers/ProvidedNameMapper.php (1)
__construct(11-11)
src/Objects/Serializers/JsonSerializer.php (7)
tests/Unit/SerializerTests/JsonSerializerTest.php (1)
MapName(67-75)src/Objects/Serializers/AbstractSerializer.php (3)
AbstractSerializer(11-69)serialize(50-56)deserialize(58-64)src/Objects/Serializers/Mappers/CamelCaseMapper.php (1)
map(13-16)src/Objects/Serializers/Mappers/IPropertyMapper.php (1)
map(11-11)src/Objects/Serializers/Mappers/PascalCaseMapper.php (1)
map(13-16)src/Objects/Serializers/Mappers/ProvidedNameMapper.php (1)
map(13-16)src/Objects/Serializers/Mappers/SnakeCaseMapper.php (1)
map(13-16)
src/Objects/Serializers/Attributes/MapOutputName.php (4)
src/Objects/Serializers/Attributes/MapInputName.php (2)
Attribute(12-18)__construct(15-17)src/Objects/Serializers/Attributes/MapName.php (2)
Attribute(16-29)__construct(19-28)src/Objects/Serializers/Mappers/ProvidedNameMapper.php (1)
__construct(11-11)tests/Unit/SerializerTests/JsonSerializerTest.php (6)
__construct(19-23)__construct(28-34)__construct(39-44)__construct(49-53)__construct(58-64)__construct(70-74)
tests/Unit/SerializerTests/JsonSerializerTest.php (7)
src/Objects/Serializers/JsonSerializer.php (3)
JsonSerializer(19-190)serialize(36-43)deserialize(46-53)src/Objects/Serializers/Mappers/ProvidedNameMapper.php (2)
ProvidedNameMapper(9-17)__construct(11-11)src/Objects/Serializers/Mappers/SnakeCaseMapper.php (1)
SnakeCaseMapper(11-17)src/Objects/Serializers/Attributes/MapInputName.php (1)
__construct(15-17)src/Objects/Serializers/Attributes/MapName.php (1)
__construct(19-28)src/Objects/Serializers/Attributes/MapOutputName.php (1)
__construct(15-17)src/Objects/Serializers/AbstractSerializer.php (2)
serialize(50-56)deserialize(58-64)
src/Objects/Serializers/Mappers/IPropertyMapper.php (4)
src/Objects/Serializers/Mappers/CamelCaseMapper.php (1)
map(13-16)src/Objects/Serializers/Mappers/PascalCaseMapper.php (1)
map(13-16)src/Objects/Serializers/Mappers/ProvidedNameMapper.php (1)
map(13-16)src/Objects/Serializers/Mappers/SnakeCaseMapper.php (1)
map(13-16)
🪛 PHPMD (2.15.0)
src/Objects/Serializers/Mappers/ProvidedNameMapper.php
13-13: Avoid unused parameters such as '$propertyName'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (20)
src/Objects/Serializers/Attributes/MapInputName.php (1)
12-18: LGTM!Clean attribute implementation that follows PHP attribute best practices and maintains consistency with
MapOutputName.src/Objects/Serializers/Mappers/IPropertyMapper.php (1)
9-12: LGTM!Clean interface definition that establishes a solid contract for property name mapping implementations.
src/Objects/Serializers/Mappers/SnakeCaseMapper.php (1)
11-17: LGTM!Clean mapper implementation that correctly delegates to
Str::snake()and follows the established mapper pattern.src/Objects/Serializers/Mappers/CamelCaseMapper.php (1)
11-17: LGTM!Clean mapper implementation that correctly delegates to
Str::camel()and follows the established mapper pattern.src/Objects/Serializers/Mappers/ProvidedNameMapper.php (1)
9-17: LGTM!The implementation correctly returns a fixed mapped name regardless of input, which is the intended behavior for custom field name mapping. The
readonlyclass modifier is excellent for immutability. The static analysis warning about unused$propertyNameis a false positive—the parameter must exist to satisfy theIPropertyMapperinterface contract, but this mapper intentionally ignores it to return the provided name.src/Objects/Serializers/Attributes/MapOutputName.php (1)
12-18: LGTM!Clean attribute implementation that follows PHP attribute best practices and maintains consistency with
MapInputName.src/Objects/Serializers/Mappers/PascalCaseMapper.php (1)
11-17: LGTM!Clean mapper implementation that correctly delegates to
Str::studly()for PascalCase conversion and follows the established mapper pattern.src/Objects/Serializers/Attributes/MapName.php (1)
19-28: LGTM!The constructor logic correctly defaults
$outputto$inputwhen not provided, which is sensible default behavior for symmetric mapping scenarios.src/Objects/Serializers/JsonSerializer.php (7)
9-22: LGTM! Clean imports and constants.The new imports support the MapName attribute functionality, and the direction constants are clear and well-named.
36-43: LGTM! Good backward compatibility pattern.The routing logic cleanly separates MapName-aware serialization from the default Symfony serializer behavior.
45-53: LGTM! Consistent routing pattern.The deserialization routing follows the same clean pattern as serialization.
58-77: LGTM! Comprehensive attribute detection.The method correctly checks both class-level and property-level attributes with early return optimization.
141-151: LGTM! Clear precedence logic.These methods correctly implement property-level attribute precedence over class-level attributes using the null coalescing operator.
168-189: LGTM! Correct attribute precedence and direction handling.The method correctly prioritizes direction-specific attributes (MapInputName/MapOutputName) over MapName, and properly handles the fallback logic where MapName's output defaults to input when not specified.
79-99: Verify if nested objects with MapName attributes require special handling.The
serializeWithMapName()method serializes property values directly viajson_encode($data)without recursively callingserialize()on nested objects. This means any nested objects with MapName attributes won't have their mappings applied—they'll use PHP's default serialization behavior.However, the test suite contains no cases with nested objects (all test properties are scalar types: string, int). Before addressing this, confirm whether nested object serialization is a requirement for your use case.
Optional: Improve error diagnostics by including
json_last_error_msg()in the exception message.tests/Unit/SerializerTests/JsonSerializerTest.php (5)
2-12: LGTM! Complete test imports.All necessary attribute and mapper imports are present for the comprehensive test coverage.
26-75: LGTM! Comprehensive test object coverage.The test objects cover all major mapping scenarios: property-level vs class-level attributes, separate input/output mapping, custom mappers, and various mapper types.
98-108: LGTM! Validates MapName serialization.Test correctly verifies that properties with MapName(SnakeCaseMapper) are serialized with snake_case field names.
110-118: LGTM! Validates MapName deserialization.Test correctly verifies that snake_case JSON fields are mapped back to camelCase properties.
120-210: LGTM! Excellent test coverage.The remaining test cases provide comprehensive coverage of:
- Separate input/output mapping with different field names
- Custom ProvidedNameMapper for explicit field names
- Class-level MapName attribute application
- MapName with explicit input/output parameters
- Fallback to Symfony serializer when no MapName attributes present
All tests are well-structured and validate both serialization and deserialization paths.
Summary by CodeRabbit
New Features
Tests