Skip to content

add convenience implementations of metamodel types#485

Merged
otaviojava merged 5 commits intojakartaee:mainfrom
gavinking:metamodel-impl
Feb 29, 2024
Merged

add convenience implementations of metamodel types#485
otaviojava merged 5 commits intojakartaee:mainfrom
gavinking:metamodel-impl

Conversation

@gavinking
Copy link
Member

Rather than requiring that every implementation of Jakarta Data supply implementations of the metamodel interfaces (which are almost certainly going to be mutually isomorphic), I think it would be really convenient if the Jakarta Data jar came with these three extremely trivial record types built in.

This is especially useful for those of us who have impls based 100% on code generation, and would prefer to avoid introducing a runtime support module.

WDYT?

Copy link
Member

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

It certainly makes sense to have a common implementation of these, but there is one significant drawback around maintaining backward compatibility. We already know that we are planning to add more components to these records after 1.0 release. This is fine, however if these are spec API, which they are even though it is in a package named impl then applications could be using the existing constructor directly and we would need to maintain backward compatibility by preserving a AttributeRecord(String Name) constructor even after adding more components. This is all achievable - Java lets you do that. The problem is that we end up with API in which one of the constructors allows for initializing only a subset of attributes, which is awkward and allows for what are essentially partially initialized instances where we will need to figure out what to do for the other components. They could be left null or could be made to raise exceptions when invoked. This can all be done, but we might be better off just not putting ourselves into that position. I tried to think of another way to do this, such as not using records and having a method like Attribute.of(name) to create them, but it reduces to the same problem where we can never get rid of the partial initialization that this would open up.

Copy link
Member

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

Oops, never mind my previous comment. I didn't look closely enough at your changes. You are doing

record SortableAttributeRecord<T>(String name)

and then implementing all other components/accessor methods based on the single name value, rather than

record SortableAttributeRecord<T>(String name, Sort<T> asc, Sort<T> desc)

I'm not sure why I read it the wrong way first.

Your approach completely avoids the problem I was thinking about. What you have will work and is a good idea.

One aspect to consider will be the tradeoff between creating new Sort instances on demand and pre-initializing them in the constructor. The latter approach would optimize toward lots of repeated usage, but has the drawback of unnecessary creation for all the components that aren't used. I don't know which is better so I thought I'd bring it up to ensure it gets thought about. There is a lot of potential for repeated use, but also a lot of potential for parts of the model never being touched.

@gavinking
Copy link
Member Author

There is a lot of potential for repeated use, but also a lot of potential for parts of the model never being touched.

I think this is probably one of those things where it's better to pay the cost of using something only when you actually use it. And, as you say, I'm guessing that most attributes never get sorted.

I guess in principle we could lazy init a volatile, but that sort of code just looks really ugly to my eyes.

gavinking added a commit to gavinking/data that referenced this pull request Feb 25, 2024
gavinking added a commit to gavinking/data that referenced this pull request Feb 25, 2024
gavinking added a commit to gavinking/data that referenced this pull request Feb 25, 2024
gavinking added a commit to gavinking/data that referenced this pull request Feb 25, 2024
@njr-11
Copy link
Member

njr-11 commented Feb 26, 2024

@otaviojava you should review this as well

gavinking added a commit to gavinking/data that referenced this pull request Feb 26, 2024
gavinking added a commit to gavinking/data that referenced this pull request Feb 26, 2024
gavinking added a commit to gavinking/data that referenced this pull request Feb 26, 2024
Copy link
Member

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

I just remembered to review copyrights on new files

gavinking and others added 4 commits February 28, 2024 09:54
…java

Co-authored-by: Nathan Rauh <nathan.rauh@us.ibm.com>
…java

Co-authored-by: Nathan Rauh <nathan.rauh@us.ibm.com>
…eRecord.java

Co-authored-by: Nathan Rauh <nathan.rauh@us.ibm.com>
…ord.java

Co-authored-by: Nathan Rauh <nathan.rauh@us.ibm.com>
gavinking added a commit to gavinking/data that referenced this pull request Feb 28, 2024
gavinking added a commit to gavinking/data that referenced this pull request Feb 28, 2024
gavinking added a commit to gavinking/data that referenced this pull request Feb 28, 2024
gavinking added a commit to gavinking/data that referenced this pull request Feb 28, 2024
otaviojava added a commit that referenced this pull request Feb 29, 2024
similar to #485, add convenience impls of Page and Slice
@otaviojava otaviojava merged commit 4fc8a93 into jakartaee:main Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants