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

JPA @Converter autoApply setting not honoured when set to false #1777

Closed
KilgallonJ opened this issue Jul 30, 2019 · 7 comments
Closed

JPA @Converter autoApply setting not honoured when set to false #1777

KilgallonJ opened this issue Jul 30, 2019 · 7 comments

Comments

@KilgallonJ
Copy link

@KilgallonJ KilgallonJ commented Jul 30, 2019

The system I'm working on has a bunch of legacy data where boolean values have been stored as 'Y' and 'N'. New tables use a BIT column instead and simply store 0 and 1. No table mixes the two approaches.

To support the legacy tables we have the following converter:

@Converter(autoApply = false)
public class BooleanToStringConverter implements AttributeConverter<Boolean, String> {

    private Logger.ALogger Log = Logger.of(BooleanToStringConverter.class);

    @Override
    public String convertToDatabaseColumn(final Boolean attribute) {
        Log.debug("Converting the boolean value {}", attribute);

        if (attribute == null) {
            return "N";
        }

        return attribute ? "Y" : "N";
    }

    @Override
    public Boolean convertToEntityAttribute(final String dbData) {
        return "Y".equalsIgnoreCase(dbData);
    }
}

As this only needs to apply to certain entities the autoApply property has been set to false.

I'm now creating a brand new entity, with a new table. It has two boolean properties, both using the BIT column style instead of Y/N:

@Entity
@Table(name = "MyEntity")
public class MyEntity {

    @Id
    @Column(name = "MyEntityId")
    private Long id;

    @Column(name = "IsClosed")
    private Boolean closed;

    ...
}

Note that I have not applied the @Convert annotation

I have a query that needs to filter out any rows where the entity is closed:

query.where().eq(CLOSED, Boolean.FALSE)

It is at this point that my problem arises. Whenever this query is run I see the log message from the BooleanToStringConverter being written to the logs and indeed, if I dump the actual SQL that was executed from the MySQL database then I can see that the converter did actually get applied to the boolean property, creating the following SQL fragment:

select <columns>
  from MyEntity t0
 where <other predicates>
   and t0.IsClosed = 'N'
 order by <order clause>

This is obviously wrong - the converter shouldn't have been applied, it's not set to be automatic and the closed property isn't annotated with @convert.

I tried to work around this by creating a second converter:

@Converter(autoApply = true)
public class BooleanConverter implements AttributeConverter<Boolean, Boolean> {

    private Logger.ALogger Log = Logger.of(BooleanConverter.class);

    @Override
    public Boolean convertToDatabaseColumn(final Boolean attribute) {
        Log.debug("Processing the value {}.", attribute);
        return attribute;
    }

    @Override
    public Boolean convertToEntityAttribute(final Boolean dbData) {
        return dbData;
    }
}

This resulted in both converters being applied to the property and I see both debug statements appearing in the logs.

2019-07-29 14:19:53,994 [dispatcher-69] DEBUG BooleanConverter     Processing the value false.
2019-07-29 14:19:53,994 [dispatcher-69] DEBUG BooleanToStringConve I'm Converting the boolean value false

Next I tried explicitly setting the converter to use on the entity itself (I hoped this might change the order that the converters were getting applied in so that it'd end up as true/false despite the other converter running):

@Entity
@Table(name = "MyEntity")
public class MyEntity {

    @Id
    @Column(name = "MyEntityId")
    private Long id;

    @Convert(converter = BooleanConverter.class)
    @Column(name = "IsClosed")
    private Boolean closed;

    ...
}

With exactly the same result; both converters are applied to the value sequentially, with the BooleanToStringConverter having the last laugh and mangling the predicate.

I would rather keep the BooleanToStringConverter as it makes dealing with the legacy data a bit less painful, but unless I can figure out why it's being applied when it shouldn't it's looking likely that I'll have to delete it.

I'm using Ebean version 4.1.3 and Play! 2.6.21

@rbygrave

This comment has been minimized.

Copy link
Member

@rbygrave rbygrave commented Aug 7, 2019

Hi @KilgallonJ ... just checking that you have a workaround / are not blocked by this. I recall you don't strictly need this now, is that right?

@KilgallonJ

This comment has been minimized.

Copy link
Author

@KilgallonJ KilgallonJ commented Aug 7, 2019

That's correct @rbygrave - I removed the offending converter as it turned out to be a pretty straightforward thing to do so it's not causing me any grief at the moment.

@rbygrave

This comment has been minimized.

Copy link
Member

@rbygrave rbygrave commented Aug 19, 2019

Note that I'm pondering the potential of NOT fixing this issue.

That is, currently Ebean has pretty much "universal types" meaning that any given scalar type has a single mapping. We sort of let java.util.Date have multiple mappings but that is imo undesirable and not nice (and we should be using the java.time types now anyway).

For example Boolean has a single mapping to a DB type for a given Database/EbeanServer instance. That is, Ebean doesn't current support mapping Boolean to both Y/N AND also map Boolean to True/False for a given EbeanServer instance (we only allow 1 mapping for Boolean type).

The nice thing about this approach we have with Ebean (the simplicity we get from this) is that for example, whenever Ebean binds a Boolean it knows how it map it to the DB regardless of the property in the expression. We use this in various places like DTO etc.

To fix this we lose that simplicity which opens Ebean up for more complexity and potential confusion.

In short I think we should put this on hold until there is more clear need for mapping a single type specifically for given properties (and that we want to own the downsides of that).

@KilgallonJ

This comment has been minimized.

Copy link
Author

@KilgallonJ KilgallonJ commented Aug 19, 2019

From my point of view the main use-cases that I've come across are those cases when you're dealing with inconsistent database representations of the same type of data. It's not strictly limited to boolean values but is often one of the culprits.

In an ideal world you wouldn't need the converter for something so trivial, but changing the type of an in-use column on a production system isn't done lightly. In my case I actually can fix it at source as a major section of the app is being redesigned from the ground up.

Being able to "turn off" an attribute converter by specifying autoApply = false allows you to migrate from the old representation to the new one with minimal pain as you can selectively control the conversion of the db values.

There's also the matter of developer expectation - coming from a Hibernate background I didn't think twice about using autoApply = false because it works as expected there. It's not unreasonable to expect that it would work in another JPA implementation.

@rbygrave

This comment has been minimized.

Copy link
Member

@rbygrave rbygrave commented Aug 19, 2019

It's not strictly limited to boolean

Quite right. I was just using it as an example and yes I didn't make that clear.

allows you to migrate from the old representation to the new one with minimal pain

Yes and yes this is a nice feature. It just comes at the cost of extra complexity (so we need to make sure we want to pay that complexity cost because everyone pays for it forever).

developer expectation - coming from a Hibernate background I didn't think twice about using autoApply = false because it works as expected there

Yes that's true. What we also need to be mindful that Hibernate is approx 7MB and they have approx 2800 open JIRA tickets.

I still think we should hold on fixing this until there is more demand. We can detect autoApply = false ... throw and exception and point to this ticket.

@rbygrave

This comment has been minimized.

Copy link
Member

@rbygrave rbygrave commented Aug 19, 2019

It's not unreasonable to expect that it would work in another JPA implementation.

True as well. Note that this would be only the 2nd JPA mapping feature we don't support per say.

@rbygrave

This comment has been minimized.

Copy link
Member

@rbygrave rbygrave commented Aug 20, 2019

Ok, closing this. People who desperately need this functionality will need to voice their opinion and push for a re-open etc.

@rbygrave rbygrave closed this Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.