Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

subset of entity attributes - Find methods #921

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

njr-11
Copy link
Contributor

@njr-11 njr-11 commented Jan 2, 2025

This PR contains some of the work for #539
It covers returning one or more entity attributes rather than the entire entity when using the Find annotation.
(A separate PR will be needed to cover selecting multiple entity attributes in JDQL and corresponding update to @Query methods to allow a record return type there).

This PR will enable users to write code like:

@Repository
public interface Cars extends BasicRepository<Car, String> {
    @Find
    @Select(_Car.PRICE)
    Optional<Float> getPrice(String vin);
} 

and

@Repository
public interface Cars extends BasicRepository<Car, String> {
    record ModelInfo(String make,
                     String model,
                     int year) {}

    @Find
    Optional<ModelInfo> getModelInfo(String vin);
} 

or for the case where the record component names don't match the entity attribute names,

@Repository
public interface Cars extends BasicRepository<Car, String> {
    record ModelInfo(String model,
                     String manufacturer,
                     int designYear) {}

    @Find
    @Select({_Car.MODEL, _Car.MAKE, _Car.YEAR})
    Optional<ModelInfo> getModelInfo(String vin);
} 

or an alternative way of doing the above by placing the annotation onto the record component(s) instead (which is convenient if you have control over the definition of the record and it is used by multiple repository methods),

@Repository
public interface Cars extends BasicRepository<Car, String> {
    record ModelInfo(String model,
                     @Select(_Car.MAKE) String manufacturer,
                     @Select(_Car.YEAR) int designYear) {}

    @Find
    Optional<ModelInfo> getModelInfo(String vin);
} 

@njr-11 njr-11 added the enhancement New feature or request label Jan 2, 2025
@njr-11 njr-11 added this to the 1.1 milestone Jan 2, 2025
@gavinking
Copy link
Contributor

gavinking commented Jan 2, 2025

This approach looks basically OK to me, though I have a couple of comments.

The first is that we need a way to specify the entity type when not using BasicRepository. This is what I would put in the @Find annotation. And I would work really hard to leave other stuff out of @Find. I would use a dedicated annotation for projections. So we could write:

@Repository
public interface Cars {
    record ModelInfo(String model,
                     String manufacturer,
                     int designYear) {}

    @Find(Car.class) 
    @Project({_Car.MODEL, _Car.MAKE, _Car.YEAR})
    Optional<ModelInfo> getModelInfo(String vin);
} 

The second comment is that I would like the option of putting @Project on the fields of the record:

@Repository
public interface Cars  {
    record ModelInfo(@Project(_Car.MODEL) String model,
                     @Project(_Car.MAKE) String manufacturer,
                     @Project(_Car.YEAR) int designYear) {}

    @Find(Car.class)
    Optional<ModelInfo> getModelInfo(String vin);
} 

@gavinking
Copy link
Contributor

i.e. I want to cut a very wide berth around horrible stuff like:

    @Find(from = Car.class, value = {_Car.MODEL, _Car.MAKE, _Car.YEAR}) 
    Optional<ModelInfo> getModelInfo(String vin);

Let's not box ourselves into something like that.

@njr-11
Copy link
Contributor Author

njr-11 commented Jan 2, 2025

The first is that we need a way to specify the entity type when not using BasicRepository. This is what I would put in the @Find annotation. And I would work really hard to leave other stuff out of @Find. I would use a dedicated annotation for projections. So we could write:

@Repository
public interface Cars {
    record ModelInfo(String model,
                     String manufacturer,
                     int designYear) {}

    @Find(Car.class) 
    @Project({_Car.MODEL, _Car.MAKE, _Car.YEAR})
    Optional<ModelInfo> getModelInfo(String vin);
} 

That's a good idea. Having the value of @Find be the entity class would eliminate the need to define a primary entity type for the repository. I'm fine with having a separate annotation to optionally specify the projection, and @Project works as a name for it. Another name that I would consider for it is @Select which would match the JDQL syntax for achieving the same thing. And I suppose another advantage of separating out the projection to its own annotation is that it would open up the possibility of using it in combination with a partial JDQL query that omits the SELECT clause.

The second comment is that I would like the option of putting @project on the fields of the record

That's a nice improvement as well. This allows for more concise code because it isolates specifying the entity attribute names to a single place (the record) rather than each repository method where it is returned. Additionally, for records where all of the component names match except one, it could allow overriding only that one.

i.e. I want to cut a very wide berth around horrible stuff like:

    @Find(from = Car.class, value = {_Car.MODEL, _Car.MAKE, _Car.YEAR}) 
    Optional<ModelInfo> getModelInfo(String vin);

Let's not box ourselves into something like that.

Totally agree. We should never add multiple fields to @Find.

I'll get the PR updated.

@gavinking
Copy link
Contributor

Another name that I would consider for it is @Select which would match the JDQL syntax

That would be fine, but I usually try to avoid "select" just because selection is really the job of the where clause. But I agree that confusion is very unlikely here.

@otaviojava
Copy link
Contributor

Once we can archive it using JPQL, do we need a duplicate version to find the annotation?

Using JPQL is:

  • Easier than finding annotation (we don't need an extra annotation to link this projection)
  • Requires less cognitive load
  • Most users are already familiar with the one from JPA.

Once it is a subset, we could use a similar structure:

@Repository
public interface Cars {
    record ModelInfo(String model,
                     String manufacturer,
                     int designYear) {}

    @Query("select new ModelInfo(model, manufacturer, year) from Car")
    Optional<ModelInfo> getModelInfo(String vin);
} 

@gavinking
Copy link
Contributor

gavinking commented Jan 3, 2025

@otaviojava I think it's nice to be able to have both.

    @Find(Car.class) 
    @Project({_Car.MODEL, _Car.MAKE, _Car.YEAR})
    Optional<ModelInfo> getModelInfo(String vin);

vs

    @Query("select model, make, year from Car where vin = ?1") 
    Optional<ModelInfo> getModelInfo(String vin);

And:

    @Find(Book.class) 
    @Project(_Book.Title)
    List<String> getModelInfo(Restriction<Book> restriction, Order<Book> order);

vs

    @Query("select title from Book") 
    List<String> getModelInfo(Restriction<Book> restriction, Order<Book> order);

I agree that in these cases the JDQL versions look slightly cleaner, but the @Project versions don't look terrible to my eyes, and they're a bit more type-safe for people who aren't using an annotation processor to validation the JDQL.

@njr-11
Copy link
Contributor Author

njr-11 commented Jan 3, 2025

PR and description are updated to split out the entity attribute names and use the Find value for the entity type instead.

Copy link
Contributor

@gavinking gavinking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks very reasonable. Left minor comments.

spec/src/main/asciidoc/repository.asciidoc Outdated Show resolved Hide resolved
@@ -26,11 +26,25 @@


/**
* <p>Annotates a repository method returning entities as a parameter-based automatic query method.</p>
* <p>Annotates a repository method to be a parameter-based automatic query method
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, but I find "Annotates X to be Y" really clumsy English.

It's fine to write "Declares X to be Y", but I think "Annotates X as Y" was also perfectly OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like I rewrote that sentence when switching around two parts of it. I'll get that corrected back to "as" instead of "to be"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected in d93b48e along with removal of an extra "a" that I spotted in a different section that I had updated.

api/src/main/java/jakarta/data/repository/Find.java Outdated Show resolved Hide resolved
Copy link
Contributor

@gavinking gavinking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional comments.

api/src/main/java/jakarta/data/repository/Find.java Outdated Show resolved Hide resolved
api/src/main/java/jakarta/data/repository/Find.java Outdated Show resolved Hide resolved
api/src/main/java/jakarta/data/repository/Find.java Outdated Show resolved Hide resolved
njr-11 and others added 3 commits January 3, 2025 10:37
* int year) {}
*
* &#64;Find
* Optional&lt;ModelInfo&gt; getModelInfo(@By("vin") String vehicleIdNum);
Copy link
Contributor

@otaviojava otaviojava Jan 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I enjoy the @gavinking approach on this find one:

#921 (comment)

   @Find(Car.class) 
   @Project({_Car.MODEL, _Car.MAKE, _Car.YEAR})
   Optional<ModelInfo> getModelInfo(String vin);

Thus, I centralized the "builder" into a single place.

Plus, I can use a regular class instead of record, for example.

* <li>a Java record type representing a subset of entity attributes returned
* by the query. The names of the record components and entity attributes
* must match, or the entity attribute names must be specified by the
* {@link Select} annotation.</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also use a regular class here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also use a regular class here?

With a little bit of added complexity, this approach could certainly be extended to include non-record classes as well. However, I would propose that in this PR we stick with record only to start out with, get that working, and then separately consider if we want to add in non-record classes. Would that be okay with you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Nathan.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Baby-steps.
I like this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants