Support for Additional Converters by Default

[SethFalco]

Adds default converts for the following types:

• java.awt.Color
• java.awt.Dimension
• java.awt.Point
• java.io.File
• java.net.InetAddress
• java.net.URI
• java.net.URL
• java.time.Duration
• java.time.Instant
• java.time.Period
• java.util.Locale
• java.util.UUID
• java.util.regex.Pattern

Edit: Originally a request to add a plugin to the docs, but made this a contribution instead.

[tandraschko]

In general i wonder why dont you contribute directly to DeltaSpike?

[SethFalco]

I've moved everything from the proposed plugin into the DeltaSpike Core Impl module.

It includes implementations for several standard Java types, tests for each converter, and a small documentation update to mention the additional supported types.

Does this look alright so far for a PR?

[tandraschko]

looks very good to me!
@struberg can we merge it?

[SethFalco]

Since opening this PR, I've noticed Apache BeanUtils has many converters similar to how this project does. 🤔

Would it be feasible to have a general "converters" project, and have both DeltaSpike and BeanUtils depend on it?

https://github.com/apache/commons-beanutils/tree/master/src/main/java/org/apache/commons/beanutils2/converters

[tandraschko]

Not sure if both projects are interessing in such a shared codebase. Couldnt we reuse beanutils here? cc @struberg

[SethFalco]

If possible, it could be nice to get an answer on this.
I'm hoping the pull request to commons-beanutils could be merged soon.

• If DeltaSpike may depend on BeanUtils: we can close this, and I may look if I can add BeanUtils in a separate PR here.
• If DeltaSpike won't depend on BeanUtils: I'll rebase/update this to make improvements or match feedback from BeanUtils.

[struberg]

Hi Seth! I'd rather not depend on BeanUtils. The less dependencies we do have, the better it is imo. The code for picking up the converters is rather trivial.

That said, what do you think about doing something similar than we did when implementing mp-config over in Geronimo: implicit converters!
https://github.com/apache/geronimo-config/blob/ConfigJSR/impl/src/main/java/org/apache/geronimo/config/converters/ImplicitConverter.java

I've also added this to the ConfigJSR proposal: https://github.com/eclipse/ConfigJSR/blob/master/spec/src/main/asciidoc/converters.asciidoc#implicit-converters
With that we'd need to add Converters only for those classes where the implicit converter rules do not already apply. Wdyt?

[SethFalco]

Hey! Understood, I'll squash this PR and match the improvements suggested from BeanUtils.
Even if this won't be merged as is, the logic should still be updated anyway.

Meanwhile, having implicit converters seems cool.

This PR includes 5 converters that would be made redundant, assuming we stuck with the current implementations.

• Duration (Duration#parse)
• File (File::new)
• Instant (Instant#parse)
• Period (Period#parse)
• URI (URI::new)

Just a note, but something that could've been implicit, but doesn't meet the current spec is #fromString(java.lang.String)?
But only the following have it in the standard library:

• UUID (UUID#fromString)
• PosixFilePermissions (PosixFilePermissions#fromString)

[SethFalco]

Just finished cleaning this up, a summary of the changes:

• Squashed the commits.
• Reduced the verbosity of the docs/errors. (Semantics are the same, they were just really wordy.)
• Used more concise syntax for testing exceptions. (@Test(expected = {}) (Comment)
• Updated @since 1.9.5 to @since 1.9.6 since 1.9.5 has already been released.
• Duration, Instant, and Period no longer accept Long values by default. (Comment)
• Updated tests for URL to compare the URI instead to avoid depending on an internet connection.
• Allow British and American spellings for Color. (Comment)

[rmannibucau]

think we must take care of the packages, we likely don't want awt package to be loaded there because there is a security manager or javaagent for example.
think it can be better to provide the classes and enable to provide a Map<Class, Converter> to the resolver.
by default - for implicit resolvers - we can try to load all the ones we support - even if it should be per package group likely to avoid to fail 4 times when once is enough?. This default loading should likely be done in the config extension or something like that and cached to be reused/reusable (if the extension is injected). Manual resolvers can be loaded without any additional converter (primitives/wrappers being the one we must support OOTB only IMHO).

Side note: for the story behind this way to do, we did the exact same thing in xbean with its converts == load them all by default, but in practise it is very bothering and letting them be configurable and loaded at demand only is way more relevant on user land in general.

[SethFalco]

Only took me 2.5 years to come back to this. ^-^'

In general, this makes sense. I'm currently trying to set up DeltaSpike v2.0.0-SNAPSHOT for development, so I can clean this up and finally get it in a state it can be merged. I've encountered an issue that I wanted to ask about, though.

In a sandbox project, it looks like OpenWebBeans is trying to find and inject a java.util.regex.Pattern, rather than leaving it for the DeltaSpike Configuration module to handle. 🤔

I don't see any immediate issues with how things are set up, though. Sorry to ask, but are you aware of any breaking changes that may prevent this from working:

https://gist.github.com/SethFalco/82e16523963880c609b3a9b09ac6fd67

Meanwhile, I've rebased and fixed issues raised by CI. 👍🏽

[rmannibucau]

I guess it is cause you didn't set a converter on the injection and there is no default built-in producer of such a type (this is the challenge with this PR, to modify org.apache.deltaspike.core.impl.config.ConfigurationExtension#collectDynamicTypes for default case when there is a known converter in the converter "registry" but avoid to load and define everything upfront which would make the whole container slower for a very poor gain for most apps)

[SethFalco]

Ahh, right this makes a little more sense to me now.

we likely don't want awt package to be loaded there because there is a security manager or javaagent for example

I've at least handled this part.

For now, the existing converters (primitives) are all supported out of the box, while the new proposed converters can be loaded on demand with a producer.

@Produces
public static PatternConverter getPatternConverter() {
    return new PatternConverter();
}

I'm now able to use DeltaSpike + these converters as I'd expect when imported into another project.

provide a Map<Class, Converter> to the resolver

Could I make you elaborate on this part? Unfortunately, I did try to look around the configuration extension, but wasn't sure how to apply it.

[rmannibucau]

On the type list I don't get all the choices (ex: why file and not path, why instant but not offsetdatetime which are more human friendly) but not a big blocker but the doc should document how to use it since as this DeltaSpike does not support the new type, it enables to use them if you do the work in your app - this is why I thought they should be matched in the extension and enabled implicitly ifuser didn't registered a custom producer (converter bean actually).

That's the big picture for me, minimum there: ensure the doc explains how to use this feature and flag in the table the manual converter as such vs the implicit ones (primitive wrappers, string, class).

rest is good for me

hope it makes sense.