reintroduce static metamodel?#394
Conversation
@njr-11 I would love it if you guys could find time to talk me through this. In JPA it's totally the responsibility of the provider to initialize fields of the metamodel, and so the metamodel interfaces are just interfaces. But it looks like you're proposing something a bit different here. That's fine, but I would like to understand how it impacts existing metamodel generators and interop with JPA. |
Relying on the provider was the original plan, and is still an option. In this case the provider needs to initialize it if it wasn't already initialized (indicated by the Generated annotation). I believe Otavio was thinking that some IDEs would offer a way to define and generate the fully implemented metamodel during development time. @otaviojava and @gavinking Would it work to move the Jakarta Data meeting from Wednesday, Dec 20 to be at the same time this week on Wednesday, Dec 13? We could use it to sort out what should happen with the static metamodel and whether some of it should be included in version 1.0. |
Yeah right that's the way it works in JPA.
Yeah so I would like to understand the thinking there.
I can make it, the only thing is I have a hard cutoff at the 55 minute mark. |
@otaviojava If this works for you as well (it does for Gavin and myself), go ahead and move the Jakarta Data meeting to Dec 13 (I'll make sure to log on early enough to check if I need to start it up) for discussion. If not, we can aim to discuss at the scheduled meeting on the following week. |
Sorry, I saw the message now. Did you talk? |
njr-11
force-pushed
the
230-reintroduce-static-metamodel
branch
from
a9a7818 to
51b8821
Compare
|
Another point, IMHO: a good point to start should be the Jakarta Persistence API. But, not to this version, to the following version. |
Otavio - we did not have any meeting to discuss this last week because you did not respond beforehand and you need to be there to work out with Gavin what is acceptable and which release it will be in. We can discuss it this week instead. The update that I just pushed is only a rebase onto latest, not any additional changes. |
|
Folks, do you think we could set up a time to talk through this after the xmas period? |
There is a Jakarta Data meeting scheduled for Wednesday, January 3, where we would discuss this if both of you are available for it. |
Signed-off-by: Nathan Rauh <nathan.rauh@us.ibm.com>
njr-11
force-pushed
the
230-reintroduce-static-metamodel
branch
from
51b8821 to
346a8a7
Compare
|
The latest updates to the proposed metamodel reflect changes that were discussed at the January 3 meeting. |
Co-authored-by: Otávio Santana <otaviopolianasantana@gmail.com>
|
@otaviojava and @KyleAure This pull is updated with the review comment that you spotted as well as the JavaDoc correction that we noted on the Jakarta Data meeting earlier today. |
|
@otaviojava could you complete your review of this so that we can merge it to enable Kyle to move forward with creating Milestone 3? |
As I said, I will need more time to test it deeply.
|
| * <li>{@link TextAttribute textual attributes}</li> | ||
| * </ul> | ||
| */ | ||
| public interface SortableAttribute extends Attribute { |
There was a problem hiding this comment.
IMHO: by default, all attributes are sortable because the database defines this aspect, not the Java programming.
I mean, I don't have any scenario where the Java side will define if the attribute is sortable or not.
There was a problem hiding this comment.
Hrrrm. Good point. That's probably right. I mean, I bet we could find some sort of data store where binary data is not sortable, but I guess we need to model that in the API.
| * | ||
| * <pre> | ||
| * @StaticMetamodel(Person.class) | ||
| * public class _Person { |
There was a problem hiding this comment.
I know that Person_is already taken from Jakarta Persistence.
Can we use another suffix instead of having a prefix?
I mean, for us, is more natural, using IDE, explore suffix at the code, for example, given a Person in a code we might have:
- PersonEntity
- PersonService
- PersonRepository
- PersonController
In general, by conversion, we work mostly with suffixes instead of prefixes. Some suggestions:
- Person_Metadata
- Person__
- PersonMetadata
There was a problem hiding this comment.
My motivation for suggesting the prefix _ was that I figured it was actually more ergonomic in the IDE: i.e. it's nicer to type _Bo<ctrl-space> than Bo<ctrl-space>_.
If you guys really hate the prefix underscore, I would suggest something like Entity_Person.
There was a problem hiding this comment.
My motivation for suggesting the prefix
_was that I figured it was actually more ergonomic in the IDE: i.e. it's nicer to type_Bo<ctrl-space>thanBo<ctrl-space>_.If you guys really hate the prefix underscore, I would suggest something like
Entity_Person.
My own preference is that I liked the single underscore because it also makes the name as concise as possible, making the code easier to read and understand,
page = people.findByAgeBetween(20, 65, Pageable.ofSize(10).sortBy(_Person.lastName.asc(),
_Person.firstName.asc(),
_Person.id.asc());
rather than
page = people.findByAgeBetween(20, 65, Pageable.ofSize(10).sortBy(Person_Metadata.lastName.asc(),
Person_Metadata.firstName.asc(),
Person_Metadata.id.asc());
There was a problem hiding this comment.
🤔 I did not know that. Thank you for explaining.
If it is more ergonomic to the IDE, let's keep this way.
| /** | ||
| * Represents an entity attribute in the {@link StaticMetamodel}. | ||
| */ | ||
| public interface Attribute { |
There was a problem hiding this comment.
I would merge this interface with SortableAttribute, I am assuming that the minimum of an attribute is being sortable.
There was a problem hiding this comment.
I would merge this interface with
SortableAttribute, I am assuming that the minimum of an attribute is being sortable.
We should not assume that every attribute is sortable. For example, the Jakarta Data specification lists byte[] as a data type that represents binary data. I would not expect that to be sortable. Similarly, if an attribute is an embeddable or collection type, it might not make sense to be able to sort it either. Having a interface to designate non-sortable types helps prevent the user during development time from introducing errors of requesting sort on things that cannot be sorted.
There was a problem hiding this comment.
Similarly, if an attribute is an embeddable or collection type, it might not make sense to be able to sort it either.
Ah yeah, of course. Forgot about that.
There was a problem hiding this comment.
I would merge this interface with
SortableAttribute, I am assuming that the minimum of an attribute is being sortable.We should not assume that every attribute is sortable. For example, the Jakarta Data specification lists
byte[]as a data type that represents binary data. I would not expect that to be sortable. Similarly, if an attribute is an embeddable or collection type, it might not make sense to be able to sort it either. Having a interface to designate non-sortable types helps prevent the user during development time from introducing errors of requesting sort on things that cannot be sorted.
@njr-11 My point is that the database will define what is sortable or not, non the spec or the Java App.
There was a problem hiding this comment.
@njr-11 My point is that the database will define what is sortable or not, non the spec or the Java App.
I agree. The database makes the decision. The Java App must be aware of that -- what is and is not sortable -- in order to write valid code asking to sort on some attributes and not others. If the Java App tries to declare something sortable that cannot be sorted, that should result in an error. Maybe we should write that somewhere in the spec.
| * // These can be uninitialized and non-final if you don't need to access them from annotations. | ||
| * public static final String SSN = "ssn"; | ||
| * public static final String NAME = "name"; | ||
| * public static final String NAME_FIRST = "name.first"; |
There was a problem hiding this comment.
What happens with Entity with relationship?
For example,
class Person {
private Address address;
}
class Company {
private Address headquarter;
}
class Address {
private String street;
}Thus, the metadata:
class PersonMetadata {
public static final String Address_STREET = "address.street";
}
class CompanyMetadata {
public static final String HEADQUARTER_STREET = "headquarter.street";
}
class AddressMetadata {
public static final String STREET = "street";
}Please ignore the terminology. My point is. This constant might have a substantial recursive constant.
As much as we have a relationship, it might grow. E.g.: person.address.country.city.street
I would skip this constant.
For example, Jakarta Persistence does not have this:
There was a problem hiding this comment.
What happens with Entity with relationship?
An entity only has these fields for its direct members. So no HEADQUARTER_STREET, etc.
[Note that I guess we didn't say anything about embeddables yet, but in JPA embeddables have their own static metamodel types.]
Jakarta Persistence does not have this
It's coming in JPA 3.2
There was a problem hiding this comment.
Your example mostly looks good but has an error/typo here:
class AddressMetadata { public static final String STREET = "name"; }
The value should be "street".
Please ignore the terminology. My point is. This constant might have a substantial recursive constant.
As much as we have a relationship, it might grow. E.g.:person.address.country.city.streetI would skip this constant.
Your particular hierarchy doesn't make any sense. You wouldn't find a city embeddable inside of the country or a street field on a city. Let's use person.address.city.state.country as a better example.
If the user defines their entities and embeddable objects that way, it certainly could have a lengthy relationship. But that isn't a problem of the static metamodel. The Jakarta Data specification already defines this behavior with embeddables. A user can already do this if they want to:
people.findByYearOfBirth(int birthYear, Sort.asc("person.address.city.state.country"), ...);
The only part the static metamodel is adding is allowing them to do it in a type safe way.
The example with that many levels of embeddables isn't as realistic. However, one or two levels of embeddables is something users will want to do. This is something we support in the spec, and I don't see why we should skip over having a type safe solution for it like we do for other usage.
For example, Jakarta Persistence does not have this
That's fine. We don't need to match Persistence for the sake of copying. We should include it because referencing embeddables in this way is part of the Jakarta Data programming model and we should make our whole programming model type safe.
There was a problem hiding this comment.
For the record I'm not sure this is necessary:
public static final String NAME_FIRST = "name.first";
But I need to think it through some more.
There was a problem hiding this comment.
The example with that many levels of embeddables isn't as realistic.
Unfortunately, it is more natural than we wish. But let me create a real scenario here, and I will simplify it.
Unfortunately, it happens more than I wish, for example, in e-commerce:
class Customer {
private long id;
private Address billingAddress;
private List<Order> orders;
}
class Order {
private long id;
private Customer customer;
private List<Product> products;
private Address shipping;
private Payment payment;
}
class Address {
}
class Payment {
Address address;
}So, there are two different paths for Addresses, where to client this is for billing and the order the address is to shipping the product:
billingAddress.streetshipping.street
My point is only the static and how to solve this point.
There was a problem hiding this comment.
I am fine to merge the PR, but we need to cover all scenarios once we include this metadata, furthermore, it should cover both SQL and NoSQL databases. Including the feature is the first step; make sure it works is the second step.
There was a problem hiding this comment.
About the path, I would go to the same path that we do in code as metadata, thus:
_Customer. _Address.street.asc()
There was a problem hiding this comment.
Yeah, I guess that's about right @otaviojava .... except probably this: _Customer.address.street.asc()
There was a problem hiding this comment.
Excellent - I think that's exactly how we defined it, so we just need to confirm that what we wrote is sufficiently clear.
There was a problem hiding this comment.
Disregard my comment above about that exactly matching how we defined it. I misread it as _Customer.address_street.asc() (how we defined it), but the difference is in the . vs _ between address and street. It does look nicer the way you wrote it. However, for that to work, the _Customer.address Attribute would also need to also be a static metamodel class, so it would need to have a type that is both Attribute and _Address.
@StaticMetamodel(Customer.class)
public class _Customer {
public static final String ID = "id";
public static final String BILLINGADDRESS = "billingAddress";
public static final String BILLINGADDRESS_STREET = "billingAddress.street";
public static volatile SortableAttribute id;
public static volatile _Address billingAddress; // <-- Both Attribute and _Address
}
@StaticMetamodel(Address.class)
public class _Address implements Attribute { // <-- added implements
public static final String STREET = "street";
public static volatile SortableAttribute street;
@Override
public String name() { // <-- who provides/implements the Attribute methods?
}This approach seems to make things more complex and has some issues that would need to be worked through. It might be possible for the metamodel class for the embeddable to become abstract and then avoid declaring the Attribute methods, but ultimately the method does need to be implemented, and wherever that happens will run into the problem that the name() is different depending on which entity embeds it, because they can each name the Address field whatever they like.
| /** | ||
| * Represents an textual entity attribute in the {@link StaticMetamodel}. | ||
| */ | ||
| public interface TextAttribute extends SortableAttribute { |
There was a problem hiding this comment.
I would remove this interface and keep the static metamodel as simple as possible.
There was a problem hiding this comment.
I would remove this interface and keep the static metamodel as simple as possible.
If we consolidate it all onto SortableAttribute, then anything that is a SortableAttribute will be able to request .ascIgnoreCase() and .descIgnoreCase() which means letting users write nonsense code for numeric fields. By having these specific types, it makes a hierarchy of interfaces that control what you can actually do with the attributes based on what types they are. That is a superior model because it stops errors from being introduced while code is being developed.
|
it is became huge. |
4 participants
This is where we left off at on the static metamodel. At one point, it was removed and deferred to next release, which was supposed to have been with the approval of @gavinking who had opened the original issue for it: #230 but based on recent comments in that issue I suspect that was missed and didn't happen.
If we do decide to bring this back, this pull includes all of the latest that we had for static metamodel, including the working TCK tests. If I recall correctly, one of the items that remained under discussion was whether we should keep the pattern where the metamodel fields are static (as in this pull) versus switch to one where the fields are volatile. The former is safer, but required additional code in the spec to facilitate it. This was discussed in a meeting with Gavin on January 3, 2024 and it was decided to go with the non-final/volatile approach for metamodel attributes that is consistent with Jakarta Persistence. Also discussed during this meeting - to avoid conflicts on generated class names with Jakarta Persistence, the convention was switched to place the underscore first, and also bring back the ability to have String fields on the metamodel for name values that can be used within annotations.