Adding CRUD Annotations for Dynamic Repositories

[otaviojava]

This Pull Request (PR) proposes the addition of new annotations for dynamic Repositories within the Jakarta Data framework: Insert, Delete, Update, and Save. These annotations simplify and enhance the interaction with data entities in dynamic Repository interfaces. They provide a more intuitive way to declare methods for various CRUD (Create, Read, Update, Delete) operations.

Benefits:

1. Improved Code Readability: The new annotations make it easier to identify the intended operation of each method within dynamic Repositories, improving code readability and maintainability.

2. Consistency Across Operations: Each annotation enforces a specific operation type, ensuring consistency in method declarations and reducing potential errors.

3. Enhanced Documentation: The Javadoc documentation for these annotations offers comprehensive explanations, helping developers understand their usage and behavior.

4. Type-Safe Returns: The annotations provide type safety by ensuring that the return type matches the parameter type, enhancing code quality.

Sample Dynamic Repository:

As an example, consider a dynamic Repository interface for a Garage that manages Car entities:

@Repository
interface Garage {

    @Insert
    Car park(Car car);  // Insert a car entity and return it

    @Delete
    void unpark(Car car);  // Delete a car entity

}

In this example, the @Save annotation indicates that the park method performs an insert operation and returns the inserted Car entity. Similarly, the @Update, @Delete, and @Save annotations indicate their respective operations.

Usage:

Developers can now use these annotations to create dynamic Repository interfaces that are more expressive and self-documenting, leading to cleaner and more maintainable code.

[otaviojava]

@gavinking @njr-11 it still WIP, I hope finishi until our meeting today.

[graemerocher]

I think this is reasonable, though would still expect a method called save or delete to just do the right thing

[njr-11]

I think this is reasonable, though would still expect a method called save or delete to just do the right thing

I agree with Graeme. This pattern is reasonable, but unnecessary.

It would also prevent us from being able to better define these annotations for other purposes in the future, so I am not in favor of it.

If the consensus of the group is to put these changes in now, there are a couple of minor errors in the JavaDoc that will need correcting. I do appreciate that you've put in the effort to fully specify the requirements that the user will need to know within the JavaDoc.

[njr-11]

It turns out there were more issues than I realized in the JavaDoc. Hopefully I found all of them.

[otaviojava]

@njr-11

[njr-11]

It looks like you missed some review comments from before (Github probably collapsed/hid them) and I added a couple of new ones, at least one of which is my fault from introducing a grammar mistake (left out the word "the") in one of my previous suggestions.

[njr-11]

Recommend switching the exception type from IllegalStateException to UnsupportedOperationException

Looking at the JavaDoc for IllegalStateException would imply that the operation is valid, but being invoked at the wrong time:

Signals that a method has been invoked at an illegal or inappropriate time. In other words, the Java environment or Java application is not in an appropriate state for the requested operation.

That isn't what we want to convey. The point we are trying to get across with the exception is that combining multiple operation annotations isn't supported.

[otaviojava]

Recommend switching the exception type from IllegalStateException to UnsupportedOperationException

Looking at the JavaDoc for IllegalStateException would imply that the operation is valid, but being invoked at the wrong time:

Signals that a method has been invoked at an illegal or inappropriate time. In other words, the Java environment or Java application is not in an appropriate state for the requested operation.

That isn't what we want to convey. The point we are trying to get across with the exception is that combining multiple operation annotations isn't supported.

@njr-11 I mean when this code happened:

@Insert
@Update 
void operation(Car car);

Given a relational database, both have support, don't they? But is it one I will execute?
In my mind, it should be Illegal.

[njr-11]

@Insert
@Update 
void operation(Car car);

Given a relational database, both have support, don't they? But is it one I will execute?
In my mind, it should be Illegal.

I think we agree here. A combination @Insert @Update operation or combination @Save @Delete operation or whatever other combination, is not supported and so must raise an exception. I was only pointing out that the description of IllegalStateException indicates it isn't the right exception for this and I suggested UnsupportedOperationException instead, which you already added the commits for, so I think we're fine on this.

[otaviojava]

@Insert
@Update 
void operation(Car car);

Given a relational database, both have support, don't they? But is it one I will execute?
In my mind, it should be Illegal.

I think we agree here. A combination @Insert @Update operation or combination @Save @Delete operation or whatever other combination, is not supported and so must raise an exception. I was only pointing out that the description of IllegalStateException indicates it isn't the right exception for this and I suggested UnsupportedOperationException instead, which you already added the commits for, so I think we're fine on this.

I will improve the documentation, but I hold this one, mainly, because the insert and update behavior will move to here as well.

[gavinking]

I was only pointing out that the description of IllegalStateException indicates it isn't the right exception for this and I suggested UnsupportedOperationException instead, which you already added the commits for, so I think we're fine on this.

It might be worth pointing out that an implementation of Jakarta Data might also choose to detect (and report) this error at compile time.

[njr-11]

I'll agree to the same compromise here as on 287 - the entity return value is okay for Insert, but not for Update, where it is inefficient and forces extra retrieval from the database.

[otaviojava]

I'll agree to the same compromise here as on 287 - the entity return value is okay for Insert, but not for Update, where it is inefficient and forces extra retrieval from the database.

As we did with insert, how about allowing both?
So, in a case that does not need such as a mutable entity or the entity does not have any kind of version is fine.

However, if the user works with a Java record of any version control they can work with that as well.

[njr-11]

I'll agree to the same compromise here as on 287 - the entity return value is okay for Insert, but not for Update, where it is inefficient and forces extra retrieval from the database.

As we did with insert, how about allowing both? So, in a case that does not need such as a mutable entity or the entity does not have any kind of version is fine.

However, if the user works with a Java record of any version control they can work with that as well.

Yes, that's a good point, in the case of the annotation, we don't need to choose a single return type, and can be more flexible like this. I would be fine with that, but please let's use a different pull after this one goes in. It is already big enough and I think we're otherwise almost ready to merge this pull. I highlighted 3 code review comments from before (hopefully non-controversial) that hid had marked outdated but still apply.

[otaviojava]

I'll agree to the same compromise here as on 287 - the entity return value is okay for Insert, but not for Update, where it is inefficient and forces extra retrieval from the database.

As we did with insert, how about allowing both? So, in a case that does not need such as a mutable entity or the entity does not have any kind of version is fine.
However, if the user works with a Java record of any version control they can work with that as well.

Yes, that's a good point, in the case of the annotation, we don't need to choose a single return type, and can be more flexible like this. I would be fine with that, but please let's use a different pull after this one goes in. It is already big enough and I think we're otherwise almost ready to merge this pull. I highlighted 3 code review comments from before (hopefully non-controversial) that hid had marked outdated but still apply.

You are right it became huge!
Please review it and I will open a new PR to move this update discussion.

[njr-11]

You are right it became huge!
Please review it and I will open a new PR to move this update discussion.

It looks good now. Thanks!