Skip to content

Unknown column 'new' in 'field list' when implementing Persistable in a class with @AccessType(Type.PROPERTY) #2338

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

Closed
mauromol opened this issue Mar 16, 2021 · 8 comments
Assignees
Labels
type: documentation A documentation update

Comments

@mauromol
Copy link

My Spring Data JDBC entity is implementing Persistable and is annotated with @AccessType(Type.PROPERTY) at the class level.
When I invoke save on my repository, I get the following exception from the underlying JDBC implementation:

java.sql.SQLException: Unknown column 'new' in 'field list'

Indeed, looking at the executed query, Spring Data JDBC is also specifying the column new in the insert statement. I think it comes from the implementation of org.springframework.data.domain.Persistable.isNew().

I take the opportunity to ask a question: is it true that implementing Persistable allows me to avoid to mark the primary key field with @Id, being the implementation of org.springframework.data.domain.Persistable.getId() enough for the framework to detect the primary key? It's not very clear from the documentation.

@mp911de mp911de transferred this issue from spring-projects/spring-data-relational Mar 23, 2021
@mp911de
Copy link
Member

mp911de commented Mar 23, 2021

Moved to Spring Data Commons as the functionality is originating from Commons.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 23, 2021
@mp911de
Copy link
Member

mp911de commented Mar 23, 2021

That is actually intended behavior. Implementing Persistable requires the implementation of getId and isNew which are then considered property accessors for the id and new properties. You can annotate either method with @Transient to exclude that property from your entity.

@mauromol
Copy link
Author

mauromol commented Mar 23, 2021

Hmmm... how can this be the intended behaviour? If the purpose of Persistable.isNew() is to determine whether the entity is new or not, it can't be considered "by default" a normal property to be persisted on the database... it's a contradiction!

Any valid and reasonable use case in which the user would like to persist the "being new" status of an entity implementing Persistable (which will most probably be always true or always false on save, depending on whether the check is made before or after the relevant fields used to implement isNew() have been populated or not)?

@mauromol
Copy link
Author

mauromol commented Mar 23, 2021

The use of @Transient is a valid workaround, however if Spring Data requires it to be used with Persistable.isNew() I think it's playing bad on this specific case.
Persistable is a special interface used to orchestrate persistence for entities with certain requirements. Persistable.isNew() then is not an actual property, but rather a utility method to determine whether the entity is new (= never persisted before) or not. The fact that its name is in the form of an accessor method is IMHO "incidental".
As in my previous message, I really can't think of a reasonable case in which it would be desirable to store this new property in the database. And perhaps, if one would really like to do it, perhaps they might use an internal isNew field, with @AccessProperty(FIELD), which could be computed accordingly and returned by Persistable.isNew(), so an opt-in mechanism for a very special (and weird) use case rather than an opt-out mechanism to make the framework behave in a "least-surprise" way...

@schauder
Copy link
Contributor

The reason we decided to go this way is exactly to make it least surprising. This way the rules are actually fairly simple:

Attributes are determined by fields. You can change that to properties by setting the access type PROPERTY.
If you want something that is detected as an attribute not to be persisted you may annotate it with @Transient.

It would be much weirder if either id and new behave differently from each other, or adding an interface would make the id property disappear one of which we would have to do if we would decide to make your case a little more easier.
And then you get into the question how to make them "untransient" if you want to.
All this while your case is already an edge case: Very few people use Persistable and @AccessType(Type.PROPERTY) together.

@mauromol
Copy link
Author

Persistable.getId() is a different thing and it's natural to consider it a property to persist, because of its semantic. I was arguing against Persistable.isNew() only.

I still believe this is an unfortunate decision, although I respect it. Persistable is a special interface and I think it should deserve a special treatment, at least for the isNew() method, since it's obvious (by its semantic) that it should not be intended as a property to persist.

Regarding the use of Persistable and @AccessType(Type.PROPERTY) together: I think the latter one is not so frequently used because the default field access type "just works", however documentation suggests that using property access should be better from a performance point of view. This is why I added it to my class, considering it had the required accessor methods already.
Perhaps Persistable belongs to Spring Data Commons, but I'm using it with Spring Data JDBC. I never needed to use it with Spring Data JPA, probably because in JPA there are better ways to determine whether an entity is new or not. But in Spring Data JDBC it's absolutely required unless you use generated primary keys on all your entity classes. Still, Spring Data JDBC mentions Persistable super-quickly (just in one table row within the whole document; see: https://docs.spring.io/spring-data/jdbc/docs/current/reference/html/#jdbc.entity-persistence.state-detection-strategies). Just to say that they are probably seldom used (especially in combination) because they are not so much promoted, rather than because they constitute an edge case...

And then you get into the question how to make them "untransient" if you want to.

Yes, but... you still didn't mention any reasonable use case in which storing the result of Persistable.isNew() would be desirable ;-) Once again, the key difference here is the semantic of that method.

Anyway, I acknowledge your decision.

@schauder
Copy link
Contributor

But in Spring Data JDBC it's absolutely required unless you use generated primary keys on all your entity classes.
This is not true, see https://stackoverflow.com/a/50385071/66686

@mauromol
Copy link
Author

Oh, well, and where the @Version thing would be documented?
See: https://docs.spring.io/spring-data/jdbc/docs/current/reference/html/#jdbc.entity-persistence.state-detection-strategies

Indeed, this is what I'm doing in my Persistable.isNew() implementation (i.e. checking whether version is 0), but nowhere I read this is done automatically by Spring Data when a @Version property exists.

UPDATE: oh, well, I see it's mentioned at https://docs.spring.io/spring-data/jdbc/docs/current/reference/html/#jdbc.entity-persistence.optimistic-locking, however it's like a treasure hunt! :-D I would suggest you to add the information written in the second place to the table at the first link...

schauder pushed a commit that referenced this issue Mar 25, 2021
schauder pushed a commit that referenced this issue Mar 25, 2021
schauder pushed a commit that referenced this issue Mar 25, 2021
schauder pushed a commit that referenced this issue Mar 25, 2021
@schauder schauder added this to the 2.2.14 (Moore SR14) milestone Mar 25, 2021
schauder added a commit to spring-projects/spring-data-relational that referenced this issue Mar 26, 2021
The use of a version property to determine the state of an entity wasn't properly documented so far.

Related tickets spring-projects/spring-data-commons#2338
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation A documentation update
Projects
None yet
4 participants