Skip to content

Treat no attributes annotated with @ConvertedWith as association. #2168

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
myfrontpocket opened this issue Mar 4, 2021 · 6 comments
Closed
Assignees
Labels
type: documentation A documentation update type: enhancement A general enhancement

Comments

@myfrontpocket
Copy link

myfrontpocket commented Mar 4, 2021

With 6.0.5, I'm trying to work through some OGM-to-SDN migration, and I think I may have come across an issue with @ConvertWith, but I'm not sure if my usage is correct or not.

I have a repository test that tests a findById method, but as the test spins up and sets up I get an error: java.lang.IllegalStateException: Required identifier property not found for class ...

The generalized code:

@Node
public class DomainOne {

	@Id
	@Property
	@GeneratedValue(GeneratedValueStrategy.class)
	private String id;
	
	@ConvertWith(converter = DomainTwoConverter.class)
	private DomainTwo domainTwo = new DomainTwo();
}

@Getter
@Setter
public class DomainTwo {
	
	private boolean aBooleanValue;
	private Long aLongValue;

	public DomainTwo() {
		aLongValue = 0;
	}

	public DomainTwo(boolean aBooleanValue, Long aLongValue) {
		this.aBooleanValue = aBooleanValue;
		this.aLongValue = aLongValue;
	}

}

@Repository
public interface DomainOneRepository extends Neo4jRepository<DomainOne, String> {
	
	@Query("MATCH (domainOne:DomainOne { id: $id })"
		// plus OPTIONAL MATCH statements
		"RETURN domainOne;")
	Optional<DomainOne> findById(String id);
}


public DomainTwoConverterClass implements Neo4jPersistentPropertyConverter<DomainTwoConverterClass> {

	private static final A_BOOLEAN_VAUE = "aBooleanValue";
	private static final A_LONG_VALUE = "aLongValue";

	@Override
	public Value write(DomainTwo source) {
		Map<String, Object> values = Map.of(
			A_BOOLEAN_VAUE, source.aBooleanValue(),
       		A_LONG_VALUE, source.aLongValue()
    	);
    	
    	return Values.value(values);
	}

	@Override
	public DomainTwo read(Value source) {
		boolean aBooleanValue = Optional.ofNullable(source.asMap().get(A_BOOLEAN_VALUE))
			.filter(obj -> obj instanceof Boolean)
			.map(Boolean.class::cast)
			.orElse(false);

		Long aLongValue = Optional.ofNullable(source.get(A_LONG_VALUE))
        	.filter(obj -> Number.class.isAssignableFrom(obj.getClass()))
        	.map(obj -> ((Number) obj).longValue())
        	.orElse(0L);

        return new DomainTwo(aBooleanValue, aLongValue);

	}
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 4, 2021
@michael-simons michael-simons self-assigned this Mar 23, 2021
@michael-simons
Copy link
Collaborator

Would you mind sharing your GeneratedValueStrategy that you are using vor @GeneratedValue? Thanks.

@myfrontpocket
Copy link
Author

myfrontpocket commented Mar 23, 2021

  private static final Integer ID_LENGTH = 8;
  private static final Pattern ID_REGEX = Pattern.compile("^[A-Za-z0-9]{8}$");
  private static final String LOWER = "abcdefghijklmnopqrstuvwxyz";
  private static final String UPPER = LOWER.toUpperCase();
  private static final String NUMERIC = "0123456789";

  @Override
  public Object generateId(Object object) {
    if (SomeObject.class.isAssignableFrom(object.getClass())) {
      SomeObject someObject = (SomeObject) object;
      if (!Optional.ofNullable(someObject.getId()).isPresent()) {
        return Optional.ofNullable(generateId())
            .filter(id -> isValidId(id))
            .orElseThrow(() -> new IllegalStateException(
            String.format("Failed to generate id for object: <%S>", someObject.toString())));
      } else {
        return someObject.getId();
      }
    }
    throw new IllegalStateException("Object does not extend SomeObject. No identifier generated.");
  }

  public String generateId() {
    String source = StringUtils.join(UPPER.concat(LOWER).concat(NUMERIC));
    StringBuilder output = new StringBuilder(ID_LENGTH);

    ThreadLocalRandom rand = ThreadLocalRandom.current();

    while (output.length() < ID_LENGTH) {
      output.append( source.charAt( rand.nextInt(source.length())));
    }
    return output.toString();
  }

@michael-simons
Copy link
Collaborator

Thanks. I just wanted to make sure that this is the reason for your issues.

So the problem is you cannot use ConvertWith to create map of properties. You must use CompositeProperty for that

public final class DomainTwoCompositePropertyConverter implements Neo4jPersistentPropertyToMapConverter<String, DomainTwo> {

	private static final String A_BOOLEAN_VALUE = "aBooleanValue";
	private static final String A_LONG_VALUE = "aLongValue";

	@Override
	public Map<String, Value> decompose(DomainTwo property, Neo4jConversionService neo4jConversionService) {
		Map<String, Value> values = new HashMap<>();
		values.put(A_BOOLEAN_VALUE, Values.value(property.isABooleanValue()));
		values.put(A_LONG_VALUE, Values.value(property.getALongValue()));
		return values;
	}

	@Override
	public DomainTwo compose(Map<String, Value> source, Neo4jConversionService neo4jConversionService) {
		boolean aBooleanValue = Optional.ofNullable(source.get(A_BOOLEAN_VALUE))
				.map(Value::asBoolean)
				.orElse(false);

		Long aLongValue = Optional.ofNullable(source.get(A_LONG_VALUE))
				.map(Value::asLong)
				.orElse(0L);

		return new DomainTwo(aBooleanValue, aLongValue);
	}
}

using like

this

@CompositeProperty(converter = DomainTwoCompositePropertyConverter.class)
private DomainTwo customConverterDefaultPrefix = new DomainTwo();

this will however generate a prefix, which is inline with Neo4j-OGM's CompositeAttributeConverter, so that shouldn't be an issue in your migration.

@michael-simons
Copy link
Collaborator

Here's test code for the example above

DomainOne domainOne = new DomainOne();
domainOne.setCustomConverterDefaultPrefix(new DomainTwo(true, 4711L));
domainOne = domainOneRepository.save(domainOne);

try (Session session = driver.session()) {
	Node node = session
			.run("MATCH (n:DomainOne {id: $id}) RETURN n", Collections.singletonMap("id", domainOne.getId()))
			.single().get(0).asNode();
	assertThat(node.get("customConverterDefaultPrefix.aBooleanValue").asBoolean()).isTrue();
	assertThat(node.get("customConverterDefaultPrefix.aLongValue").asLong()).isEqualTo(4711L);
}

@myfrontpocket
Copy link
Author

Thanks so much! I'll give this a try!

@michael-simons michael-simons changed the title "Required identifier property not found for class" error with @ConvertWith Treat no attributes annotated with @ConvertedWith as association. Mar 23, 2021
michael-simons added a commit that referenced this issue Mar 23, 2021
…ciation.

The actuall issue here is caused by `@ConvertedWith` not indicating at
all times that a property isn't an association: The moment there is a
converter (either through the Spring context or `@ConvertedWith`) it
can't be an association anymore.

The exception `Required identifier property not found for class` was
thrown because SDN treated the annotated property as association. This
is fixed now.

The commit adds tests for this and also adds an example how to use
composite or discrete property convertes. However, in alignment with
previous SDN+OGM behavior, a prefixless decomposition of properties into
maps and composition back from them is not possible.

This closes #2168.
@michael-simons
Copy link
Collaborator

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
type: documentation A documentation update type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants