micronaut-projects / micronaut-data

Ahead of Time Data Repositories
Apache License 2.0
466 stars 197 forks source link

Micronaut Data fails on update for entities with UUIDs #821

Closed sascha-frinken closed 3 years ago

sascha-frinken commented 3 years ago

Task List

Steps to Reproduce

  1. Create a maraiadb with a table

    CREATE TABLE `test_entity`
    (
    `id`       varchar(36) NOT NULL,
    `name`     varchar(30) NOT NULL,
    `created`  datetime    NOT NULL,
    `modified` datetime    NOT NULL,
    PRIMARY KEY (`id`)
    ) ENGINE = InnoDB
    DEFAULT CHARSET = utf8mb4
    COLLATE = utf8mb4_unicode_ci;
  2. Create a micronaut 2.1.4 project with a matchin entity and repository.

Entity

@MappedEntity
class TestEntity {
    @Id
    @AutoPopulated
    UUID id

    @NotBlank
    @Max(30l)
    String name

    @DateCreated
    Instant created

    @DateUpdated
    Instant modified
}

Repository

@JdbcRepository(dialect = Dialect.MYSQL)
interface DefaultRepository extends CrudRepository<TestEntity,UUID> {

}
  1. Save an entity
def e = repository.save(
    new TestEntity().tap {
        name = "test"
    }
)
  1. Update the entity
e.name = "update"
e = repository.update(e)

Expected Behaviour

Entity should be updated without error on a table with uft8.

Actual Behaviour

Updating the entity fails with following stack trace:

Error executing SQL UPDATE: (conn=18) Invalid utf8mb4 character string: 'ACED00'
io.micronaut.data.exceptions.DataAccessException: Error executing SQL UPDATE: (conn=18) Invalid utf8mb4 character string: 'ACED00'
    at io.micronaut.data.jdbc.operations.DefaultJdbcRepositoryOperations.lambda$updateOne$8(DefaultJdbcRepositoryOperations.java:615)
    at io.micronaut.transaction.support.AbstractSynchronousTransactionManager.executeWrite(AbstractSynchronousTransactionManager.java:175)
    at io.micronaut.data.jdbc.operations.DefaultJdbcRepositoryOperations.updateOne(DefaultJdbcRepositoryOperations.java:460)
    at io.micronaut.data.jdbc.operations.DefaultJdbcRepositoryOperations.update(DefaultJdbcRepositoryOperations.java:452)
    at io.micronaut.data.runtime.intercept.DefaultUpdateEntityInterceptor.intercept(DefaultUpdateEntityInterceptor.java:43)
    at io.micronaut.data.intercept.DataIntroductionAdvice.intercept(DataIntroductionAdvice.java:80)
    at io.micronaut.aop.chain.MethodInterceptorChain.proceed(MethodInterceptorChain.java:82)
    at io.micronaut.validation.ValidatingInterceptor.intercept(ValidatingInterceptor.java:139)
    at io.micronaut.aop.chain.MethodInterceptorChain.proceed(MethodInterceptorChain.java:82)
    at com.example.FailingSpec.test(FailingSpec.groovy:47)
Caused by: java.sql.SQLTransientConnectionException: (conn=18) Invalid utf8mb4 character string: 'ACED00'
    at org.mariadb.jdbc.internal.util.exceptions.ExceptionFactory.createException(ExceptionFactory.java:79)
    at org.mariadb.jdbc.internal.util.exceptions.ExceptionFactory.create(ExceptionFactory.java:153)
    at org.mariadb.jdbc.MariaDbStatement.executeExceptionEpilogue(MariaDbStatement.java:274)
    at org.mariadb.jdbc.ClientSidePreparedStatement.executeInternal(ClientSidePreparedStatement.java:229)
    at org.mariadb.jdbc.ClientSidePreparedStatement.execute(ClientSidePreparedStatement.java:149)
    at org.mariadb.jdbc.ClientSidePreparedStatement.executeUpdate(ClientSidePreparedStatement.java:181)
    at org.apache.tomcat.jdbc.pool.StatementFacade$StatementProxy.invoke(StatementFacade.java:114)
    at io.micronaut.data.jdbc.operations.DefaultJdbcRepositoryOperations.lambda$updateOne$8(DefaultJdbcRepositoryOperations.java:611)
    ... 9 more
Caused by: org.mariadb.jdbc.internal.util.exceptions.MariaDbSqlException: Invalid utf8mb4 character string: 'ACED00'
    at org.mariadb.jdbc.internal.util.exceptions.MariaDbSqlException.of(MariaDbSqlException.java:34)
    at org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.exceptionWithQuery(AbstractQueryProtocol.java:194)
    at org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.exceptionWithQuery(AbstractQueryProtocol.java:177)
    at org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.executeQuery(AbstractQueryProtocol.java:321)
    at org.mariadb.jdbc.ClientSidePreparedStatement.executeInternal(ClientSidePreparedStatement.java:220)
    ... 13 more
Caused by: java.sql.SQLException: Invalid utf8mb4 character string: 'ACED00'
    at org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.readErrorPacket(AbstractQueryProtocol.java:1688)
    at org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.readPacket(AbstractQueryProtocol.java:1550)
    at org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.getResult(AbstractQueryProtocol.java:1513)
    at org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.executeQuery(AbstractQueryProtocol.java:318)
    ... 14 more

Environment Information

Example Application

https://github.com/safri-net/mn-data-issue-uft8

Just follow the README.md to see how to reproduce the error and that everything works fine with micronaut 1.3.7.

issmo commented 3 years ago

This is linked to the way MySQL/MariaDB are handling UUID. I faced the same issues for inserts and submited a PR for it. But it looks like there are still a few spots missing to completely solve the issue.

@graemerocher Setting the queries parameters is a bit scatted in different locations (inserts/updates...) and this maybe deserve a rewrite. What do you think?

graemerocher commented 3 years ago

Which area in particular needs a rewrite?

issmo commented 3 years ago

Which area in particular needs a rewrite?

Mainly the DefaultJdbcRepositoryOperations.java and AbstractSqlRepositoryOperations.java files and pertinent to prepared statement parameters setting. There are a couple of locations that I identified and that might contains such edge case where a specific Dialect handling is made in one of them and not the others

Maybe this would not be feasible, but I think it is worth documenting this.

graemerocher commented 3 years ago

Yes it is worth pointing out differences

sascha-frinken commented 3 years ago

@graemerocher I just created a quick & dirty fix (tests are still passing). If you want to have a look at it -> https://github.com/safri-net/micronaut-data/tree/2.1.x.

Short description I am passing the dialect and when the dialect requires UUID as String I don't use setValue but setString

I can reate a PR if you want.

sascha-frinken commented 3 years ago

For anyone interested our workaround so far is to change all UUID columns charset to latin1

alter table xx modify col varchar(36) character set latin1
sascha-frinken commented 3 years ago

I have to correct my self - it has nothing to do with charset. Updating an entity with a uuid property always fails (silently).

This is from the mysql query log:

UPDATE `table` SET `name`='test' WHERE (`id` = _binary '\xAC\xED\ sr\ java.util.UUID\xBC\x99\xF7\x98m\x85/\ J\ leastSigBitsJ\ mostSigBitsxp\xB0\xA3\x9D\xE3\"\xF0\xAC\xB6]\xE1ZxN\xE1')

As you can see the UUID is not converted to a string.

This is a blocker for us and I hope it gets some attention from the micronaut team.

@graemerocher If you could give some feedback to my changes (https://github.com/safri-net/micronaut-data/tree/2.1.x see here) I am pleased to help you with a PR.

issmo commented 3 years ago

@sascha-frinken As Mysql/MariaDb requires UUID to be of string type, you can decorate the id of your entities with @ TypeDef(type = DataType.STRING) and everything should work as expected.

sascha-frinken commented 3 years ago

@issmo Thanks for the hint. I will give it a try. I just submitted a PR - I hope that at least my tests will get accepted…

issmo commented 3 years ago

@sascha-frinken can you confirm this solves your issue

sascha-frinken commented 3 years ago

@issmo yes I can confirm annotating UUID properties with @TypeDef(type = DataType.STRING) is a working workaround.

But if this is required by micronaut it should get some attention in the docs and it should be mentioned as a breaking change.

issmo commented 3 years ago

@sascha-frinken I still think this is to be considered as a bug. At least the framework has enough flexibility to match the specificities of the different databases types implementation.

graemerocher commented 3 years ago

So yes @TypeDef(type = DataType.STRING) is the right thing to do since the changes involve making breaking API changes I think it should be either documented that this the thing to do.

As a bug fix it may make it nicer if there was some logic that defaulted to @TypeDef(type=DataType.String) if the dialect is mariadb or mysql. I think this should be done at the compilation phase though.

issmo commented 3 years ago

@graemerocher I think there might some cases where defaulting at compile-time to @TypeDef(type=DataType.String) wouldn't be correct. The @TypeDef might be on an entity that could be used by different repositories each with a different dialect. I admit this is probably an edge case... but who knows

sascha-frinken commented 3 years ago

@issmo I just had a look at this and I have to admit that compilation phase isn't really my comfort zone… I am pretty sure that you need the repository to get this done as this is the only place that holds the dialect - or am I wrong?

sascha-frinken commented 3 years ago

@graemerocher As I already mentioned, I am not experienced in doing compilation phase stuff. I would really love to help with a PR, but I would definitely need some advice where to start, and I think @issmo is right - we need to discuss the problem that an entity could be used in different repos with different dialects…