Checking for possible composite converters in GraphEntityMapper.writeProperty

[oxisto]

This allows to resolve conflicts when a property is using a composite converter.

Fixes #932

[meistermeier]

Would really like to have a separate conditional branch with a dedicated message, maybe even more visible level, here.

Suggested change

	if (writer == null || writer.isComposite()) {
	logger.debug("Unable to find property: {} on class: {} for writing", property.getKey(), classInfo.name());
	} else {
	if (writer == null) {
	logger.debug("Unable to find property: {} on class: {} for writing", property.getKey(), classInfo.name());
	} else if (writer.isComposite()) {
	logger.info("Property is already handled by a CompositeAttributeConverter"); // or similar wording

[oxisto]

Yes that makes sense. Just FYI, for other properties that are handled by the CompositeAttributeConverter, which do not have the name conflict, the method gets also called and the writer is actually null. Therefore, the original debug message might also be misleading, but we can probably not differentiate between "somehow we could not find the property" and "property is already handled" in the first if.

[meistermeier]

Thanks for the PR, could you also provide a test case for this?
Something that maybe checks for the right log output like we do here

neo4j-ogm/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/context/SingleUseEntityMapperTest.java

Line 313 in 0079fd7

assertThat(loggerRule.getFormattedMessages()).areAtLeastOne(stringMatches);

(Please don't be irritated by the ..Rule naming ;) )

[oxisto]

Thanks for the PR, could you also provide a test case for this? Something that maybe checks for the right log output like we do here

neo4j-ogm/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/context/SingleUseEntityMapperTest.java

Line 313 in 0079fd7

assertThat(loggerRule.getFormattedMessages()).areAtLeastOne(stringMatches);

(Please don't be irritated by the ..Rule naming ;) )

Sure, will do. Might need a little bit to find the proper test layout.

[oxisto]

Unfortunately, I am a little lost with regards to tests @meistermeier. I can find tests for the SingleUseEntityMapper (which seems to have a similar problem), but not for the GraphEntityMapper

[meistermeier]

TBH the test suite is broad but unfortunately not well sorted. Something we have to address in the future (tried this once and dismissed it ;) )
For know, add the test to the PropertyTest in the integration test module, put your domain in the org/neo4j/ogm/domain/gh932 package and add it here:

neo4j-ogm/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/persistence/types/properties/PropertiesTest.java

Line 54 in 0079fd7

sessionFactory = new SessionFactory(getDriver(), User.class.getName(), SomeNode.class.getName());

I will revisit it (or leave it there) before merge. Thanks for the effort.

[meistermeier]

Just saw your test commit: Just leave it as it is. Would also work for me.

[oxisto]

TBH the test suite is broad but unfortunately not well sorted. Something we have to address in the future (tried this once and dismissed it ;) ) For know, add the test to the PropertyTest in the integration test module, put your domain in the org/neo4j/ogm/domain/gh932 package and add it here:

neo4j-ogm/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/persistence/types/properties/PropertiesTest.java

Line 54 in 0079fd7

sessionFactory = new SessionFactory(getDriver(), User.class.getName(), SomeNode.class.getName());

I will revisit it (or leave it there) before merge. Thanks for the effort.

Ok I did that. The tests run successful on my machine. Fingers crossed for GitHub actions ;)

[meistermeier]

Locally this is already running perfectly fine. I would go ahead and merge this.
Could you please sign the CLA? https://neo4j.com/developer/cla/
I've put the log output check also in the test case, just to be sure that the new condition gets hit and not the writer==null.

[oxisto]

Locally this is already running perfectly fine. I would go ahead and merge this.
Could you please sign the CLA? https://neo4j.com/developer/cla/
I've put the log output check also in the test case, just to be sure that the new condition gets hit and not the writer==null.

Done

[meistermeier]

Thanks for signing and letting me push the changes on your branch.
This makes merging much nicer.

[oxisto]

Thanks for signing and letting me push the changes on your branch.
This makes merging much nicer.

Excellent, thanks! Just wondering... Are you following a certain release schedule, e.g. every X months or are you basically releasing if enough PRs are merged? Looking forward to 4.0.2 ;)

[meistermeier]

...like this https://github.com/neo4j/neo4j-ogm/releases/tag/v4.0.2?
😺