Invoke registered converters even if types are compatible

[donalmurtagh]

I wrote the following converter which I hoped would convert phone numbers to a canonical format

@Log4j
class PhoneNumberConverter implements ValueConverter {

    @Override
    boolean canConvert(Object number) {
        number instanceof String
    }

    @Override
    Object convert(Object number) {
        '00' + number
    }

    @Override
    Class<?> getTargetType() {
        String
    }
}

I registered this in resources.groovy, but it wasn't being invoked by the databinder. I asked about this on Slack channel and @jeffbrown said

it is a scenario that hasn’t been accounted for. I would have to investigate to validate this but what I expect is happening is that the converters are only engaged if the value being bound isn’t compatible with the type of reference it is being bound to, which is not the case in your scenario. Feel free to file a feature request and we can investigate possibilities.

So it seems that converters are only invoked if the original value and the target type are incompatible. It wasn't at all obvious to me from the docs that this is the case, but it means that using converters for String -> String transformations is impossible

I'm proposing to change the behaviour, such that converters are always invoked if canConvert returns true. i.e. even if the orginal value and the target of the binding are compatible.

Rather than applying this change to all registered ValueConverter instances - which might be a breaking change - it might be preferable to introduce a new type, e.g.

public interface ValueTransformer extends ValueConverter {}

and only apply the behavioural change described above to ValueTransformer instances. You could add a default method to ValueConverter instead of introducing a new type, but of course this would only work on JDK 8.

[osscontributor]

I registered this in BuildConfig.groovy

I expect you meant resources.groovy. Is that correct?

[donalmurtagh]

Of course, I've corrected it above

[osscontributor]

We could investigate options to make something like this work but if it worked the way you are describing above, that would be problematic because that would mean every String property that was ever bound using the data binder would now have "00" prepended to it, which would break a lot of things in many apps, like plugins that are doing data binding to String properties.

[donalmurtagh]

You're right, the example above is impractical. The real implementation looks like this

import com.google.i18n.phonenumbers.NumberParseException
import com.google.i18n.phonenumbers.PhoneNumberUtil
import com.google.i18n.phonenumbers.Phonenumber.PhoneNumber
import groovy.util.logging.Log4j
import org.grails.databinding.converters.ValueConverter

@Log4j
class PhoneNumberConverter implements ValueConverter {

    private PhoneNumberUtil phoneUtil = PhoneNumberUtil.instance

    private static final IRISH_PREFIXES = ['353', '00353', '+353']

    @Override
    boolean canConvert(Object number) {
        // This converter will be called for any binding of a request param to a String command object field, i.e. a lot.
        // The libphonenumber docs suggest phone number validation is not trivial in terms of cost, so as an
        // optimization, skip any strings whose length is outside the typical range of a phone number
        if (number instanceof String && number.size() in 5..20) {
            String countryCode = getCountryCode(number)

            try {
                PhoneNumber candidateNumber = phoneUtil.parse(number, countryCode)
                phoneUtil.isValidNumber(candidateNumber)

            } catch (NumberParseException ex) {
                log.debug "Failed to convert '$number' to $countryCode phone number"
                false
            }
        }
    }

    @Override
    Object convert(Object number) {
        number = number.toString()
        String countryCode = getCountryCode(number)
        PhoneNumber candidateNumber = phoneUtil.parse(number, countryCode)
        phoneUtil.format(candidateNumber, PhoneNumberUtil.PhoneNumberFormat.E164)
    }

    private String getCountryCode(String number) {
        boolean isIrish = IRISH_PREFIXES.any { number.startsWith(it) }
        isIrish ? 'IE' : 'GB'
    }

    @Override
    Class<?> getTargetType() {
        String
    }
}

[graemerocher]

@jeffbrown what is the status on this issue, anything to do?