DefaultRecordMapper does not handle SET_ column prefix correctly

jordanamr commented 4 years ago

Expected behavior

The .getId() function from the POJO should get the correct column.

Actual behavior

The .getId() function from the POJO is returning another column ending in "id", in this case "set_id". What's most interesting is that I can see the POJO having the .getSetId(), so the code generation is probably not an issue in this case. Renaming my column to just "set" fixes the issue.

Steps to reproduce the problem

Example table structure:

Example code:

Example output:

jOOQ Gradle Plugin configuration:

Versions

jOOQ: 3.13.4
jOOQ Gradle Plugin: 5.1.0
Java: OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.8+10, mixed mode)
Database (include vendor): MariaDB 10.4.14
OS: Windows 10 2004 (19041.508)
JDBC Driver (include name if inofficial driver): org.mariadb.jdbc:mariadb-java-client:2.6.2

lukaseder commented 4 years ago

Thank you very much for your report. I can reproduce this without the DAO using this table:

CREATE TABLE getters_and_setters (
  id INT NOT NULL PRIMARY KEY,
  set_id INT
);

INSERT INTO getters_and_setters VALUES (1, 1), (2, 1), (3, 2);

And then:

Result<?> result = ctx.fetch(GETTERS_AND_SETTERS);

System.out.println(result);
System.out.println(result.into(GettersAndSetters.class));

Producing the correct result from the query, but wrong POJO mappings:

+----+------+
|  ID|SET_ID|
+----+------+
|   1|     1|
|   2|     1|
|   3|     2|
+----+------+

[GettersAndSetters (1, 1), GettersAndSetters (1, 1), GettersAndSetters (2, 2)]

lukaseder commented 4 years ago

Or also, without using the code generator:

static class SetId {
    int id;
    int setId;

    public void setId(int id) {
        this.id = id;
    }

    public void setSetId(int setId) {
        this.setId = setId;
    }
}

@Test
public void testSetId() {
    Field<Integer> id = field(name("id"), INTEGER);
    Field<Integer> setId = field(name("set_id"), INTEGER);

    SetId pojo = create.newRecord(id, setId).values(1, 2).into(SetId.class);
    assertEquals(1, pojo.id);
    assertEquals(2, pojo.setId);
}

lukaseder commented 4 years ago

Regrettably, this works "correctly" by design, see the DefaultRecordMapper Javadoc:

If Field.getName() is MY_field (case-sensitive!), then this field's value will be set on all of these (regardless of visibility):

Single-argument instance method MY_field(...)

Single-argument instance method myField(...)

Single-argument instance method setMY_field(...)

Single-argument instance method setMyField(...)

Non-final instance member field MY_field

Non-final instance member field myField

We support 2x2 styles of setters:

Setters without the set prefix
Setters with the set prefix

And then:

a. Setters using the original field name
b. Setters converting the original field name to PascalCase or camelCase (depending on the presence of the set prefix)

Only 2a is corresponding to the expected JavaBeans behaviour. Unfortunately, the styles aren't mutually exclusive, they can be combined, which is why the set_id field will be mapped to both the setId() setter (1b) and to the setSetId() setter (2b), without keeping in mind that setId() also matches the id column (2b).

This behaviour is probably wrong in most cases. I cannot think of a case where anyone would want the current behaviour. To fix this, I think there could be an incompatible change in the DefaultRecordMapper, which maintains the possibility of mixing the various styles, but allows for giving precendence to the styles, and prevents a field value from being set on several properties / setters in the target POJO.

This is quite the change with all the very subtle regression risks associated with it. I don't want to rush this without much further thought.

I can see 4 workarounds for you right now:

Use immutable POJOs, in case of which there won't be any default constructor. That would help work around the issue
Generate JPA annotations. With those present on your POJOs, the current algorithm is avoided and only the @Column annotations are used.
Use your own record mapper implementation that patches this behaviour. You can override the default like this: https://www.jooq.org/doc/3.13/manual/sql-execution/fetching/pojos-with-recordmapper-provider/
Avoid using set_ as a prefix in your column names, e.g. you could call the column card_set_id instead.

lukaseder commented 4 years ago

While this does look like a significant bug (and it is), it's intriguing to see that the current algorithm of the DefaultRecordMapper has been flawed with this issue for many years, and it does not seem to have appeared in our issue tracker yet.

Perhaps, having a set_ prefix is an edge case?

jordanamr commented 4 years ago

As I said, I changed my column name to just "set" so the issue is not a problem anymore, but it took me some time to figure out if the problem was from my code or jOOQ, if the current behaviour is keept, jOOQ should print a warning during the codegen, or completly deny it 🤔

lukaseder commented 4 years ago

I understand. These really subtle issues are a pain to debug, and we'd like to spend our time more efficiently.

I tend to prefer fixing a problem rather than implementing an elaborate warning mechanism that documents known issues. The current behaviour can't be fixed very easily, but this doesn't mean we won't fix it soon. Clearly, this shouldn't happen.

Thanks again for documenting this here.

jOOQ / jOOQ