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

WIP: Support for readonly on jakarta persistence #27084

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

OmarHawk
Copy link
Contributor

@OmarHawk OmarHawk commented Aug 26, 2024

Contains changes to more easily extend jakarta persistence configuration in @Column definition

#27048


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@OmarHawk OmarHawk force-pushed the feature/support-readonly-in-backend branch from 952f4bf to e9e006f Compare August 26, 2024 08:45
@OmarHawk OmarHawk changed the title WIP: Refactor to avoid duplicate code and WIP: Support for readonly on jakarta persistence Aug 26, 2024
@OmarHawk OmarHawk force-pushed the feature/support-readonly-in-backend branch 7 times, most recently from 6c2ea84 to e905b85 Compare August 27, 2024 09:08
@OmarHawk
Copy link
Contributor Author

OmarHawk commented Aug 27, 2024

Seems like the "IT" tests are not failing, even though they may should in the current state.

I have debugged a sample application using this change and seems like when passing a value in a non-updateable field, the save/saveAndFlush JPA Repository method still returns the set value, even though it is not what is in the (MySQL) database in the end.

I do wonder, how I can actually write a test for this kind of behaviour. Anyone having an idea, maybe @mshima ? Or do you think, we can leave this "sort of" untested this way?


According to spring-projects/spring-data-jpa#1735, this is expected behaviour.

and to more easily extend jakarta persistence configuration in Column
@OmarHawk OmarHawk force-pushed the feature/support-readonly-in-backend branch from e905b85 to 4aacc7c Compare August 27, 2024 09:12
@OmarHawk
Copy link
Contributor Author

By the way, for some reasons, it complains about some Sonar findings - but the only thing I changed there was to extend one test entity. I do wonder, if I need to/should do something about it in this Pull request

@mshima
Copy link
Member

mshima commented Aug 27, 2024

New entities added new code and increased complexity, the sonar error is not blocking.

@OmarHawk
Copy link
Contributor Author

Ok, and any hints regarding testability of these added attributes, if anything that happens in our own generated code won't notice any change due to Spring Data JPA's behaviour?

@mshima
Copy link
Member

mshima commented Aug 28, 2024

@OmarHawk readonly is quite uncommon use case.
This looks related to #25414
Use saveAndFlush for jpa and entity with fields that requires flush like @AutoGenerated?

Not sure it’s possible to test a pure @Readonly field.
Maybe another approach is required for created timestamp.
Add a @AutoGenerated which implies @Readonly annotation and test if the returned is not null?

@mshima
Copy link
Member

mshima commented Aug 28, 2024

Add readonly to typescript type?

@OmarHawk
Copy link
Contributor Author

I guess, I may finish this in around 4 weeks - heading off to some vacation. :-)

I think, the very first commit here would already be "worth" integrating, as it does only refactor the template logic for the @Column annotation parameters, but doesn't change anything to the generated applications.
The readonly support may be "extra" then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants