Skip to content

Allow custom composite converters to delete decomposed properties. #2280

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

Closed
shayantabrizi opened this issue Jun 8, 2021 · 5 comments
Closed
Assignees
Labels
type: bug A general bug

Comments

@shayantabrizi
Copy link

shayantabrizi commented Jun 8, 2021

In this code, Person has an attribute source:

    @CompositeProperty(converter = SourceConverter.class)
    Source source;

In "AService", I save a person having source, then make the source null and save it again. After retrieving the person from the database, the source attribute still has its previous value. I guess that when the updated object is saved, Neo4jPersistentPropertyToMapConverter does not add the converted attributes but the attributes are there from the previous save and no one deletes them. I checked this in a code with SDN+OGM (in which I used CompositeAttributeConverter). It seems that in there, the node didn't have the attributes. Maybe in SDN+OGM all the attributes not generated when converting the updated node were removed, but in the new version it keeps the attributes.

Here is the sample code:


        template.deleteAll(Person.class);
        Person person = template.save(new Person("John", new Source("123", "john-smith", 280L)));
        person.setSource(null);
        person.setName("Jack");
        Person updatedPerson = template.save(person);
        System.out.println("The updated person: " + updatedPerson.getName() + " " + updatedPerson.getSource() + " " +(updatedPerson.getSource() != null ? updatedPerson.getSource().getUserId() : null));
        Person savedPerson = template.findById(updatedPerson.getId(), Person.class).get();
        System.out.println("The found person: " + savedPerson.getName() + " " + savedPerson.getSource() + " " +(savedPerson.getSource() != null ? savedPerson.getSource().getUserId() : null));

and here is the converter code

    public static class SourceConverter implements Neo4jPersistentPropertyToMapConverter<String, Source> {

        @Override
        public Source compose(Map<String, Value> map, Neo4jConversionService neo4jConversionService) {
            Value userId = map.get("source_userId");
            Value userName = map.get("source_userName");
            Value size = map.get("source_size");

            if (userId != null || userName != null || size != null) {
                return new Source(
                        userId != null ? userId.asString() : null,
                        userName != null ? userName.asString() : null,
                        size != null ? size.asLong() : null
                );
            } else {
                return null;
            }
        }

        @Override
        public Map<String, Value> decompose(Source source, Neo4jConversionService neo4jConversionService) {
            Map map = new LinkedHashMap<>();
            if (source != null) {
                map.put("source_userId", source.getUserId() != null ? Values.value(source.getUserId()) : NullValue.NULL);
                map.put("source_userName", source.getUserName() != null ? Values.value(source.getUserName()) : NullValue.NULL);
                map.put("source_size", source.getSize() != null ? Values.value(source.getSize()) : NullValue.NULL);
            }
            return map;

        }
    }

And here is the result:

The updated person: Jack null null
The found person: Jack com.neo4j.sdn.Source@b20c47e2 123

How should we handle that? The updated object doesn't have a source but what I get the object from database with a source inside it.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 8, 2021
@michael-simons michael-simons self-assigned this Jun 8, 2021
@michael-simons michael-simons added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 8, 2021
@michael-simons michael-simons added this to the 6.0.10 (2020.0.10) milestone Jun 8, 2021
@michael-simons
Copy link
Collaborator

michael-simons commented Jun 8, 2021

Thanks for the great report. This is a bug and needs to be fixed in SDN. Will be in 6.0.10.

The only thing you need to do than is adapt your converter a bit

@Override
public Map<String, Value> decompose(Source source, Neo4jConversionService neo4jConversionService) {
    Map map = new LinkedHashMap<>();

     map.put("source_userId", source != null && source.getUserId() != null ? Values.value(source.getUserId()) : NullValue.NULL);
     map.put("source_userName", source != null && source.getUserName() != null ? Values.value(source.getUserName()) : NullValue.NULL);
     map.put("source_size", source != null && source.getSize() != null ? Values.value(source.getSize()) : NullValue.NULL);

    return map;
}

Values with a value of NullValue.NULL will cause the corresponding property in the graph to be deleted.

@michael-simons michael-simons changed the title Problem with @CompositeProperty Allow custom composite converters to delete decomposed properties. Jun 8, 2021
michael-simons added a commit that referenced this issue Jun 8, 2021
…erties.

Custom composite converters must be notified when a property to be decomposed is `null`.
This means we must remove the `nullSafeWrite` decorator for all composite properties first.

To be compatible with existing converters that trust us not to get a `null`, we must check for any NPE raising from such a converter.

A custom `Neo4jPersistentPropertyToMapConverter` needs to set _all_ properties it normally writes when decomposing a property to `Values.NULL`, so that they are removed from the node or relationship in question.

This fixes #2280.
michael-simons added a commit that referenced this issue Jun 8, 2021
…erties.

Custom composite converters must be notified when a property to be decomposed is `null`.
This means we must remove the `nullSafeWrite` decorator for all composite properties first.

To be compatible with existing converters that trust us not to get a `null`, we must check for any NPE raising from such a converter.

A custom `Neo4jPersistentPropertyToMapConverter` needs to set _all_ properties it normally writes when decomposing a property to `Values.NULL`, so that they are removed from the node or relationship in question.

This fixes #2280.
@shayantabrizi
Copy link
Author

Maybe, the problem is not fixed yet. After upgrading to SDN 6.1.2 I still have the problem. My sample code with updated dependencies is pushed: https://github.com/shayantabrizi/sample-code-1/tree/CompositeProperty_Test

@michael-simons
Copy link
Collaborator

Hi @shayantabrizi as I said, this https://github.com/shayantabrizi/sample-code-1/blob/CompositeProperty_Test/src/main/java/com/neo4j/sdn/Person.java#L58 is wrong.
If the source is null, you must set all the entries to null too. Please compare with my comment above. #2280 (comment) See that I move the if down.

@shayantabrizi
Copy link
Author

Oh, sorry. Thank you for that. It worked now.

@michael-simons
Copy link
Collaborator

Happy to hear 👍

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

No branches or pull requests

3 participants