ravendb / ravendb-jvm-client

RavenDB JVM Client
MIT License
13 stars 13 forks source link

Should Throw Error When No ID Field Exists/Is Specified #68

Closed cmitchell closed 1 year ago

cmitchell commented 1 year ago

Hi,

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

If your model object does not have have a Id field, and you have not set a different field to be used as the Id by calling findIdentifierProperty you don't get an error when you store the object.

In the sample code below, the Order object does NOT have an Id field. It has a uuid field, but I haven't called findIdentifierProperty. I should get an error when session.store() is calleld but I don't.

Code (Using Junit5 with Spring) The Configuration (Setup)

@Configuration
public class TestRavenConfig extends RavenTestDriver {

  @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;
  }

The Test

@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();
  }

The output -- no error is ever thrown and my uuid field (which I understand RavenDB doesn't know is what I want to use as an Id) is still null. I made sure that errors were NOT hidden by my logging level. If I had any assertions in this test, it would fail. The point is, I think this scenario should throw an error, otherwise in actual non-test code I might think everything is ok.

2023-03-06T17:08:28.379Z  INFO 11725 --- [    Test worker] net.ravendb.embedded.EmbeddedServer      : Starting global server
    2023-03-06T17:08:32.103Z  INFO 11725 --- [    Test worker] d.k.a.repository.OrderRepositoryTest     : Started OrderRepositoryTest in 5.303 seconds (process running for 10.288)

OrderRepositoryTest > save() STANDARD_OUT
    2023-03-06T17:08:32.342Z  INFO 11725 --- [    Test worker] d.k.a.repository.OrderRepositoryTest     : ===================================================
    2023-03-06T17:08:32.343Z  INFO 11725 --- [    Test worker] d.k.a.repository.OrderRepositoryTest     : uuid:
 ====================================================

OrderRepositoryTest > save() PASSED
ayende commented 1 year ago

This is by design. You are not forced to use id field, and if you don't specify one (or override the defaults), RavenDB will manage the id for you internally. You can get the document id after the store using session.advanced().getDocumentId(testOrder);

cmitchell commented 1 year ago

@ayende Thanks for the clarification. It would be more dev friendly if this were prominently documented somewhere (I culdn't find this on your website), and even better if a Warning got logged when you call store that it's being managed internally.

ayende commented 1 year ago

That is a feature, not an issue. There is a valid use case for not making the entity id as a field on the entity. This is the same as Hibernate or JPA model. For example, in Hibernate, see: https://docs.jboss.org/hibernate/orm/3.5/javadocs/org/hibernate/Session.html#getIdentifier(java.lang.Object)