Suggestion: More Intuitive Way to Specify Identifier Property

[cmitchell]

Hi,

I think I've found a few bugs regarding setting the Identifer field.

I can specify a function that returns a non-string field to be used as the Identifier when I use findIdentifierProperty. There are no checks on the function that I provide. In the code below, I tried to say I wanted to use a UUID field type as the identifier (something that is somewhat documented for C#, but not for java -- will submit a different issue ticket for this)

Code to reproduce (Using Junit 5 and Spring)

@Configuration
public class TestRavenConfig extends RavenTestDriver {

  // This allows us to modify the conventions of the store we get from 'getDocumentStore'
  @Override
  protected void preInitialize(IDocumentStore documentStore) {
     documentStore.getConventions() .setFindIdentityProperty(
             property -> property.getPropertyType().isAssignableFrom(UUID.class));
  }

  @Bean
  public IDocumentStore documentStore() {
    return getDocumentStore();
  }

  @Bean
  public SessionOptions sessionOptions() {
    SessionOptions sessionOptions = new SessionOptions();
    sessionOptions.setTransactionMode(TransactionMode.SINGLE_NODE);
    sessionOptions.setDatabase(documentStore().getDatabase());
    return sessionOptions;
  }

Test Code

@SpringBootTest(classes = TestRavenConfig.class)
@ExtendWith(SpringExtension.class)
public class OrderRepositoryTest {

  private @Autowired DocumentStore store;
  private @Autowired SessionOptions options;

  Logger logger = LoggerFactory.getLogger(OrderRepositoryTest.class);

  @Test
  public void save() {

    Order testOrder = new Order();
    IDocumentSession session = store.openSession(options);
    session.store(testOrder);
    session.saveChanges();

    [logger.info](http://logger.info/)("===================================================");
    [logger.info](http://logger.info/)("uuid: ", testOrder.getUuid());
    [logger.info](http://logger.info/)("====================================================");

    session.close();
    store.close();
  }

Output

2023-03-06T17:14:29.178Z  INFO 12542 --- [    Test worker] net.ravendb.embedded.EmbeddedServer      : Starting global server
    2023-03-06T17:14:33.007Z  INFO 12542 --- [    Test worker] d.k.a.repository.OrderRepositoryTest     : Started OrderRepositoryTest in 5.376 seconds (process running for 10.649)

OrderRepositoryTest > save() FAILED
    java.lang.IllegalArgumentException: Cannot set identity value 'orders/1-A' on field class java.util.UUID because field type is not string.
        at net.ravendb.client.documents.identity.GenerateEntityIdOnTheClient.setPropertyOrField(GenerateEntityIdOnTheClient.java:116)
        at net.ravendb.client.documents.identity.GenerateEntityIdOnTheClient.trySetIdentityInternal(GenerateEntityIdOnTheClient.java:108)
        at net.ravendb.client.documents.identity.GenerateEntityIdOnTheClient.trySetIdentity(GenerateEntityIdOnTheClient.java:88)
        at net.ravendb.client.documents.identity.GenerateEntityIdOnTheClient.trySetIdentity(GenerateEntityIdOnTheClient.java:78)
        at net.ravendb.client.documents.identity.GenerateEntityIdOnTheClient.generateDocumentKeyForStorage(GenerateEntityIdOnTheClient.java:73)
        at net.ravendb.client.documents.session.InMemoryDocumentSessionOperations.storeInternal(InMemoryDocumentSessionOperations.java:830)
        at net.ravendb.client.documents.session.InMemoryDocumentSessionOperations.store(InMemoryDocumentSessionOperations.java:786)
  net.ravendb.embedded.EmbeddedServer      : Try shutdown server gracefully.

I think the heart of the issue is that you're asking the user to know your business logic. They have to know that you're using a Function to inspect property fields (why not have them tell you the field name and you do the function), and at the time that they have to know that the Function must return a String field. I propose the following to address the above situation:

1. Deprecate findIdentityProperty. Requiring the user to pass in a function exposes a lot of your business logic to the user and honestly isn't very intuitive.

2. Replace it with one or both of the following (both hide that you're using Functions while letting the user know that only String fields are appropriate):

   • (Preferred as it is idiomatic to what other databases do) Implement an @RavenId annotation that the user would simply annotate the desired field with. That makes it clear to the user when they look at their model class which field is being used as the Id. When store() is called, check that one and only one String field is annotated and return an error if the annotated field isn't a String, if there's no annotation AND no field named "Id", or if more than one field is annotated with @Id (unless you support compound ids). Behind the scenes, set the property name of the annotated field as the argument to the function. I'm not saying RavenDB should always do what other databases do, but there are some conventions for some common tasks, like identifier specification are petty much expected. Examples from other databases
     https://mongodb.github.io/mongo-java-driver/3.7/javadoc/?org/bson/codecs/pojo/annotations/BsonId.html https://docs.spring.io/spring-data/neo4j/docs/current/api/org/springframework/data/neo4j/core/schema/Id.html

   • (less clear to the user when they are looking at thier own model) Make a setIdentifierField(String fieldName) method and have the user pass in the name of their identifier field. When store() is called, check that the object has a field with the specified name and that it's a string. Behind the scenes, set the property name of the annotated field as the argument to the function.

Both suggestions wouldn't require major changes to the underlying code -- you would just make the Function for the user. In both suggestions, in the store() method

     if (!knownTypesSet.contains(obj) {
          // do checks. 
          // make/set Function
          // add obj type to the set
     }

[ayende]

The convention function is here because you have one call for all types. You may want to say that for Orders, this is OrderId and for users it is EmployeeId, etc. Using code in this manner is the easiest way to handle all scenarios.

The requirement for identifier fields is that you'll have a string property, RavenDB explicitly do not support non string identifiers. You can provide converters to & from your id type, but RavenDB will not do that directly. See this post for the reasoning: https://ayende.com/blog/188257-B/avoiding-non-string-identifiers-in-ravendb

[cmitchell]

@ayende Yes, I understood the reasoning behind needing a String.

My point is that you could provide a more developer friendly way to specify the id field, even on multiple objects.

I.e.

Order {
   @RavenId
   private String id;
   private String address;
   etc
}

and then a different Id for Employee

Employee {
    @RavenId
     private String employeeId;
     private String  ssn;
    etc
}

Internally, you could translate to the field annotated with @RavenID to a Function something like property -> property.getAnnotations().contains("RavenDB" and that would be more inline with what other modern java databases clients do. (And not a lot of work to implement)

[ayende]

We don't want to create ties from your entities to RavenDB. The ideal is pure POJO.

Note that you can achieve the same easily enough using something like:

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.FIELD)
public @interface RavenId {
}

documentStore.getConventions() .setFindIdentityProperty(
            property -> property.isAnnotationPresent(RavenId.class));

The whole point is that you have the full flexibility at your hands, and the complexity is managable.