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

Add a way to load POJOs into Records without setting all the changed flags to true #5394

Open
lukaseder opened this issue Jul 5, 2016 · 9 comments

Comments

@lukaseder
Copy link
Member

lukaseder commented Jul 5, 2016

A lot of jOOQ users don't like the existing default behaviour of setting all Record.changed() flags to true when calling Record.from() or similar methods. The wanted behaviour is to set changed() only to true if the actual value has changed according to Objects.equals().

This could be achieved in two ways:

  1. By adding a new Setting
  2. By adding specific methods on Record

See also:

And earlier discussions:

@lukaseder
Copy link
Member Author

The change of semantics governs how the following operations work:

  • UpdatableRecord.store(), update(), insert() only copy changed values to the respective INSERT or UPDATE query, leaving the unchanged values untouched.
  • Copying record values to other records is done only for source values that are changed
  • When calling set(Record) on an Insert, Update, or Merge statement, only changed values are transferred

At least the following operations will be affected by or may be modified for a semantic shift from changed meaning "touched" to "having a modified value":

  • After UpdatableRecord.delete(), the entire record is changed(true), such that a subsequent store() call will insert the entire record again
  • Changing the primary key means that the entire record is changed(true), because we don't update primary key values by default, we only insert such modifications.
  • When records are created and values are set by jOOQ's internals (e.g. results from INSERT .. RETURNING calls), the records get a changed(false) call meaning the entire record is "untouched"

There are special cases which lie in between the two semantics

Operations that will not be affected by the change of semantics:

@lukaseder
Copy link
Member Author

A somewhat similar feature is DSLContext::fetchByExample, which delegates to DSL.condition(Record). An "example" record produces a condition from all the non-null values. This partially matches what the expectation is here, where only non-null POJO values should be copied into new records. It is not exactly the same, because if the record is not new, there may be pre-existing values in the record, in case of which setting a value to null would constitute a change of value.

@lukaseder
Copy link
Member Author

The difficult thing with changing the semantics globally is that while some operations have an available Scope, and thus Settings, others do not. For example, a statically created query could receive an non-attached record like this:

// The record is not attached, so it doesn't have a Settings, meaning that the from() call
// will set the third column to "changed", despite it being null before and after the from() call
TRecord record = new TRecord();
record.from(new TPojo(1, 2, null));

// This call also cannot access any Settings to override the default behaviour of changed(),
// so the third column will have a "null" value being inserted explicitly
Insert insert =
DSL.insertInto(T)
   .set(record)

In order to apply Settings reasonably, we'd have to make all of the above operations lazy, which is a can of worms I don't want to open for this minor feature.

Besides, even if the operations are lazy, the behaviour of toString() on Scope-free objects would resemble the current behaviour, whereas that of DSLContext.render() would be different. There are probably many other, similarly confusing differences in this already quite confusing area.

@lukaseder
Copy link
Member Author

For context: A non representative Twitter poll shows that the existing behaviour of Record types is desireable:
https://twitter.com/lukaseder/status/1232327845969629185

Setting a value to something means the intent is for that action to have a side effect in the generated query. Good reasons are:

  1. that's what jOOQ is already doing
  2. in SQL, NULL and DEFAULT are not the same
  3. in the presence of plan caches, you don't want too many different SQL strings and hard parses (at least you want control)

This issue here is about loading POJOs / DTOs into records with Record::from and similar methods. Record behaviour itself will not be modified.

@PixelBumper
Copy link

PixelBumper commented Oct 7, 2021

@lukaseder Maybe related:
I was experimenting with jooq to see how it would behave when updating a field of a record with the same value and was surprised to find out that it indeed still triggers an update

val randomUUID = UUID.randomUUID()
val account = dsl.newRecord(ACCOUNT)
account.id = randomUUID
account.email = "intial@example.org"
account.creationDate = LocalDateTime.now()
account.recVersion=1
account.insert()

val fetchedAccount = dsl.selectFrom(ACCOUNT)
    .where(ACCOUNT.ID.equal(randomUUID))
    .fetchOne() ?: throw RuntimeException()
fetchedAccount.email = "intial@example.org"
fetchedLater.store()
2021-10-07 12:37:36.397 DEBUG 96337 --- [nio-8080-exec-9] org.jooq.tools.LoggerListener            : Executing query          : update "public"."account" set "email" = ?, "rec_version" = ? where ("public"."account"."id" = cast(? as uuid) and "public"."account"."rec_version" = ?)
2021-10-07 12:37:36.399 DEBUG 96337 --- [nio-8080-exec-9] org.jooq.tools.LoggerListener            : -> with bind values      : update "public"."account" set "email" = 'intial@example.org', "rec_version" = 3 where ("public"."account"."id" = '5106fa48-cd80-404a-bf9a-43dd6fe10c79' and "public"."account"."rec_version" = 2)
2021-10-07 12:37:36.442 DEBUG 96337 --- [nio-8080-exec-9] org.jooq.tools.LoggerListener            : Affected row(s)          : 1

It would be nice to have a setting that allows to configure JOOQ to not generate an update statement for columns that do not differ from the originally fetched values.

This issue sounds at least like there could be some synergy. If not should I create a new issue for this?

@lukaseder
Copy link
Member Author

I was experimenting with jooq to see how it would behave when updating a field of a record with the same value and was surprised to find out that it indeed still triggers an update

The question you should ask yourself is, what would a SQL editor do if you "touch" a record? What would a file system do if you "touch" a record? What is the most reasonable intent of "touching" a record? The answer is always the same. It should trigger an update without changing any of the data, because the update signal itself is already useful, e.g. in the presence of triggers. Imagine a trigger that updates a LAST_MODIFIED audit field. People will want to know that you stored the record, irrespective of whether you changed anything. You locked it, you considered changing it, maybe you even did, and then changed it back to the original value within the transaction.

So much for defaults.

It would be nice to have a setting that allows to configure JOOQ to not generate an update statement for columns that do not differ from the originally fetched values.

This issue sounds at least like there could be some synergy. If not should I create a new issue for this?

I agree a setting could be useful. I'm quite sure there already is an issue, but I cannot seem to find it, so I created a new one: #12494

While there are synergies, I don't think these are the same issues. A POJO has no notion of "changed" flag, so loading a POJO into a record will always set all column values to changed.

The issue you're suggesting is to add a new notion of "modified" (or a different name) that is different from the current "changed". "modified" means "changed" && original != current (pseudo-code). That information is already useful per se, as a set of methods on Record, and then, there could be settings that govern whether we should use changed or modified for different actions, internally.

Perhaps, adding a notion of "modified", and allowing users to prefer using that over the current "changed" flag is a better way to fix what is being requested in this issue here, it might even obsolete this issue, which is purely about loading POJOs into records, an action that may continue to set all "changed" flags to true, but perhaps not all "modified" flags.

@lukaseder
Copy link
Member Author

Note, some feedback about the expected defaults for inserting/updating unchanged records can be seen here:
https://twitter.com/lukaseder/status/1281600147575775232

@PixelBumper
Copy link

The question you should ask yourself is, what would a SQL editor do if you "touch" a record? What would a file system do if you "touch" a record? What is the most reasonable intent of "touching" a record? The answer is always the same. It should trigger an update without changing any of the data, because the update signal itself is already useful, e.g. in the presence of triggers. Imagine a trigger that updates a LAST_MODIFIED audit field. People will want to know that you stored the record, irrespective of whether you changed anything. You locked it, you considered changing it, maybe you even did, and then changed it back to the original value within the transaction.

So much for defaults.

I agree with the reasoning.
And even if it were not the case, IMHO it is totally fine to have an opinionated approach regarding the default behaviour of a framework 🙂

I agree a setting could be useful. I'm quite sure there already is an issue, but I cannot seem to find it, so I created a new one: #12494

While there are synergies, I don't think these are the same issues. A POJO has no notion of "changed" flag, so loading a POJO into a record will always set all column values to changed.

👍

The issue you're suggesting is to add a new notion of "modified" (or a different name) that is different from the current "changed". "modified" means "changed" && original != current (pseudo-code). That information is already useful per se, as a set of methods on Record, and then, there could be settings that govern whether we should use changed or modified for different actions, internally.

Perhaps, adding a notion of "modified", and allowing users to prefer using that over the current "changed" flag is a better way to fix what is being requested in this issue here, it might even obsolete this issue, which is purely about loading POJOs into records, an action that may continue to set all "changed" flags to true, but perhaps not all "modified" flags.

Sounds good to me 🙂 I left some feedback regarding terminology and possible configuration options in #12494

@lukaseder
Copy link
Member Author

See also: #2704

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

No branches or pull requests

2 participants