Skip to content
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

CompositeAttributeConverter "Node model already contains property" #1190

Closed
yuiidev opened this issue Nov 6, 2024 · 4 comments
Closed

CompositeAttributeConverter "Node model already contains property" #1190

yuiidev opened this issue Nov 6, 2024 · 4 comments
Assignees

Comments

@yuiidev
Copy link

yuiidev commented Nov 6, 2024

Expected Behavior

Using multiple CompositeAttributeConverters that write to the same property does not result in naming collisions.
The properties returned from toGraphProperties and fetched by toEntityAttribute are scoped to the composite attribute name.
E.g. the model properties from an entity's uuid property will be scoped, according to this example, to uuid_most and uuid_least.

Current Behavior

Using multiple CompositeAttributeConverters that write to the same property does result in naming collisions.
The properties returned from toGraphProperties are not scoped to the composite property name resulting in naming collisions.
Changing otherUuid to use the UuidStringConverter resolves the issue, but is undesirable for me.

Context

@NodeEntity
public class MyEntity {
    @Id
    @GeneratedValue(strategy = UuidStrategy.class)
    @Convert(Uuid2LongConverter.class)
    private UUID uuid;

    @Convert(Uuid2LongConverter.class)
    private UUID otherUuid;
}
public class Uuid2LongConverter implements CompositeAttributeConverter<UUID> {
    @Override
    public Map<String, ?> toGraphProperties(UUID value) {
        var properties = new HashMap<String, Long>();
        if(value != null) {
            properties.put("most", value.getMostSignificantBits());
            properties.put("least", value.getLeastSignificantBits());
        }

        return properties;
    }

    @Override
    public UUID toEntityAttribute(Map<String, ?> value) {
        var most = (Long) value.get("most");
        var least = (Long) value.get("least");

        if(most != null && least != null) {
            return new UUID(most, least);
        }

        return null;
    }
}

This results in both properties (uuid & otherUuid) trying to convert their values to most and least properties as opposed to uuid_most, uuid_least and otherUuid_most, otherUuid_least.
Providing additional context about the property being converted to the converter would allow for the problem to be resolved.
Or alternatively, automatically scoping the returned map, providing context to the converter would allow more flexibility however; in cases where scoped properties are undesirable for example.

Your Environment

  • Java version: 21
  • Neo4j version (The version of the database): 5
  • OGM Version (The version of this software): 4.0.11
  • OGM Transport used (One of Bolt, HTTP or embedded): Bolt
  • Neo4j Java Driver version (in case of Bolt transport): 5.23.0

Steps to Reproduce

@michael-simons michael-simons self-assigned this Nov 8, 2024
@michael-simons
Copy link
Collaborator

Hej. I understand your need, and I think it makes sense to accept custom converters spotting an non-default constructor taking in a Field parameter, so that you can easily get the name or type or whatever else you need.

@michael-simons
Copy link
Collaborator

See the commit linked above, I think that should solve the use case.

Will be released as soon as we get a new version of Classgraph (unrelated case, but I hope we can avoid two releases closes by). Thanks for your valuable input / request.

@yuiidev
Copy link
Author

yuiidev commented Nov 9, 2024

This is awesome! Really good news, and solves the issue perfectly, thank you.

I'll be eagerly awaiting the release!

@michael-simons
Copy link
Collaborator

Thanks a lot for the kind feedback, 4.0.12 is out https://github.com/neo4j/neo4j-ogm/releases/tag/v4.0.12 and on Central already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants