Extension for JSR 310 Date & Time API. Fixes #33

[nicky9door]

Build note: I was able to get the whole project to compile while calling maven using JDK 1.7 by configuring a toolchain in the genson-java-datetime pom. This will require a 1.8 JDK to be configured in
~/.m2/toolchains.xml

Calling maven with a 1.8 JDK will build this module but genson-scala fails due to Java 1.8 adding additional properties to TypeVariable which are not configured in the scala classes that extend this type.
I'm not knowledgeable in scala so I'm not sure what the best solution would be

[EugenCepoi]

Let me see if we can upgrade the entire project to jdk 8. This would hopefully simplify things. Otherwise we will go with your toolchain solution, this will require however configuring travis to use it.

[EugenCepoi]

I prepared this PR to support Java 8 in the entire project.
Once I (self!) merge it you should be able to get rid of the toolchain part and your travis build should be fixed.

[nicky9door]

I merged in the changes from #122 and removed the toolchain related parts out of the pom. I had to fix one test case but everything seems to be working now

[aseovic]

This looks awesome :-)

Thank you!

[aseovic]

"desired" is misspelled

[nicky9door]

Good eye. I corrected the typo

[aseovic]

Is the annotation @TimestampFormat or @JsonTimestampFormat as the following paragraph suggests?

[nicky9door]

The timestamp is indeed @JsonTimestampFormat . I have updated the documentation

[aseovic]

@EugenCepoi @nicky9door Love it, but I do have one general, somewhat philosophical question/comment.

Considering that we are now Java 8+ only, should this code really be in a separate module (and should it even be a bundle) considering the fact that it deals with standard JDK types? I mean, we don't have a bundle for numbers, collections, and types such as URL, UUID, etc. that we have standard converters for, so why are Java 8 Date/Time types any different?

I can argue that having a bundle is a nice way to group everything together and allow easy configuration, but at the same time it feels a bit weird to treat these types as "optional" and to have to register a bundle in order for them to work.

My initial reaction is that this should be part of the core, always available, and that we also need to add support for Optionals in order to complete Java 8 support, but I'd like to hear what you guys think.

[nicky9door]

@aseovic Thanks for pointing out the issues in the documentation. I have corrected the errors you pointed out.

I originally created this as a separate bundle because genson was still using Java 1.7 at the time. Now that we are at Java 8, there is nothing really stopping us from integrating this into the core library from a technical standpoint. A possible benefit from keeping this as a separate module is that it keeps the size of the core library smaller but it does create one more dependency to remember to add to your project.

I still think we need to keep the DateTime code as bundle simply to allow users to control how these types are serialized. Though perhaps there is a way of including a default configuration while allowing users to overwrite the configuration explicitly?

[aseovic]

Yup, I get it. At the time it was the only way to do it, but now it just seems a bit strange.

I'm not worried about the size, and I know @EugenCepoi prefers not having additional JARs/dependencies, so I think merging it into the main project makes sense. I'd actually put it in com.owlike.genson.convert.time package, just to keep things clean, as there are quite a few classes there, and register all the converters along with all the other default converters in GensonBuilder.

I hear your configuration argument, but I think we can expose the "official" way to change the configuration on the GensonBuilder itself. The defaults seem to be sensible and there are also annotations that allow configuration at the individual property level (which should probably be with all other annotations, in the com.owlike.genson.annotation package), so we should be covered when it comes to configuration.

Anyway, that's just my personal opinion. Let's wait for @EugenCepoi to chime in and then we can either merge it as-is, or move things around as suggested and merge it.

[EugenCepoi]

I agree with mostly everything you guys say above.

There are already a few date related configs and annotations. I think the ideal solution would be consistent with them (reuse if possible, evolve or replace otherwise). But maybe it's not always possible, like registering only one date format and use it for datetime and the old date, etc.

If you are worried of bloating the builder with too much config you could:

• keep the bundle class it self but automatically register it. This would remain the central place to configure everything java datettime related
• or, create a DateTimeConfig class or something like that and pass it to the builder (so the user configured this DateTimeConfig instead of the builder).

I see that you have a nice readme. This doc could probably be merged with the official docs here.

Thank you guys for these great contributions :)

[aseovic]

My vote goes for option 2, DateTimeConfig class that would hold relevant configuration. We definitely don't want to bloat the builder API with additional date-related methods and add additional config fields to both GensonBuilder and Genson when they really belong elsewhere.

However, instead of adding something like GensonBuilder.withDateTimeConfig(DateTimeConfig config), which would require users to construct a new instance of config object, I suggest we modify it slightly and define GensonBuilder.withDateTimeConfig(Consumer<DateTimeConfig> config) method instead.

Basically, instead of users writing something like this:

builder.withDateTimeConfig(new DateTimeConfig().setFormatter(...))

they would do

builder.withDateTimeConfig(config -> config.setFormatter(...))

and we'd call apply(DateTimeConfig) on the consumer they provide when we need/want to.

The difference is subtle, but important, as this approach has several benefits:

1. It allows us to control when the configuration is actually applied. We can do it right away, or we can wait and do it at the right time within the create method.

2. It is composable. Multiple bundles can call the same config method on the builder, and each call will either set the consumer, if it's null, or create a composite consumer by calling Consumer.andThen. Actually, if we initialize the field to a default no-op consumer, we don't even need a null check.

3. It ensures that we have a default config object early on that we can delegate the existing useDateFormat and useDateAsTimestamp methods to, deprecate them, and remove them in 2.0 so we have only one way of configuring date handling. Otherwise, it's really tricky to control ordering and merging of those options into the new config object.

We can use the similar approach for other config options in order to clean up the builder API in 2.0 and group relevant config options together. For example, we could have ConverterConfig (converters, serializers, deserializers, factories), PropertyFilteringConfig (includes, excludes, renames), ResolverConfig (properties and naming), etc. That would leave us with a handful of well defined methods on the top-level builder, with most of the other methods moving to corresponding config objects.

Thoughts?

[EugenCepoi]

Most of it sounds good. I've been considering for some time to move bits of the builder options to a structured config. I see you considering only the advantages, let me focus on the disadvantages then :p

• Doing new DateTimeConfig() is a simpler concept and easier to understand for users on what is happening
• It depends on what you mean by "control when it is applied", but I think these config classes should be very dumb. Letting the user construct it and pass it to GensonBuilder, doesn't prevent the builder to decide when to use the config (or apply in other words).
• The builder is currently the mutable part and creates the somewhat immutable Genson instance, if we have mutable configs and pass them around as is, we potentially expose our selves to other the config changing under the hood. I don't have a better suggestion for that part though and maybe it's ok...

Could you expand a bit on 3, more specifically on the part "delegate the existing useDateFormat and useDateAsTimestamp methods to", I wonder concretely how you think the config object will help us there.

Thanks :)

[aseovic]

First, let me address disadvantages you mentioned:

1. I don't really see how

builder.withDateTimeConfig(new DateTimeConfig().setFormatter(...))

is any simpler or easier to understand than

builder.withDateTimeConfig(config -> config.setFormatter(...))

Ultimately, that's how users would see the difference, and I'd say it's 6 of one or half dozen of the other. I think it is more important that we are consistent and do it the same way for everything -- either approach is simple enough from the end user perspective.

2. As I said, the difference is subtle -- in my proposed approach we have full control over a) when the configuration object is created, and b) when the user modifications are applied to it. With new ConfigXyz() we loose both a) and b) and give user full control over creation and modification of a config object.

It may not matter much for DateTimeConfig, but it matters a lot for things where we ultimately need to be able to control ordering, such as ChainedFactory configuration, as well as PropertyNameResolver and BeanMutatorAccessorResolver configuration, which are a bit tricky to configure at the moment, but don't need to be.

For example, custom chainedFactory is at the moment always added after NullConverter, ClassMetadataConverter and RuntimeTypeConverter, which is quite limiting (and needs to be documented). If I want to insert a converter between, say, NullConverter and ClassMetadataConverter (which I actually do), my only option is to override createConverterFactory method, which is non-trivial and error prone (basically, you need to copy the existing implementation and modify it, so you don't break some of the existing default behavior).

With the approach I suggested modifying the chain becomes much simpler -- I can define a visitor, give it my standard chain once builder.create() is called, and insert additional ChainedFactories anywhere in the chain:

builder.withConverterFactory(factory -> factory.find(NullConverterFactory.class)
                                               .withNext(new SerializationSupportConverter.Factory(this)))

The code above will effectively insert my custom SerializationSupportConverter between NullConverter and ClassMetadataConverter (after I added ChainedFactory.find and made the change to ChainedFactory.withNext to allow converter factory insertion, of course, but that was easy enough to do).

The same is true for resolvers -- we need to be able to control ordering, so we can prefer our annotations to any third party annotations (JAXB, JSON-B or Jackson, for example), and we need to make sure that our standard, non-annotation based resolvers are there as a fallback at the very end.

3. There is really no difference between either approach when it comes to mutability of config objects, apart from the fact that my proposal makes it slightly harder and non-obvious to do so.

At the moment, nothing prevents you from doing:

m_dateTimeBundle = new DateTimeBundle().setFormatter(LocalDate.class, ABC);
Genson genson = new GensonBuilder().withBundle(m_dateTimeBundle).create();

// and then some time later, who knows when...
m_dateTimeBundle.setFormatter(LocalDate.class, XYZ);

Ultimately, I don't think it really matters. I think users understand (or at least they will learn the hard way) that they shouldn't modify configuration after Genson instance has been constructed and that consequences of doing so are undefined -- it may or may not work.

If we really care about that, we could provide copy constructors for all of our config objects that would ensure that whatever config objects users may end up holding a reference to are disconnected from the actual config objects Genson instance uses. That way we would ensure that they cannot change configuration after Genson instance construction, and that approach would work regardless of the approach we take.

Which brings me to my final point from the previous message, the one you seem to be most interested in ;-)

Basically, if you have only a config object and expect users to create it and pass it in, delegating existing methods to it is impossible, as the config object could be null when they are called. You'd have to have individual fields for useDateFormat and useDateAsTimestamp, and merge them into either user provided DateTimeConfig instance or the default one we'd create once GensonBuilder.create is called.

However, by separating the actual config object from its modifier, we could ensure that we always have a config modifier instance to delegate to:

private Function<DateTimeConfig, DateTimeConfig> dateTimeConfigModifier = Function.identity();  // no-op config modifier

// now we *can* do this
public GensonBuilder useDateFormat(DateFormat dateFormat) {
    this.dateTimeConfigModifier.andThen(config -> config.setFormatter(Date.class, dateFormat));
    return this;
}

public GensonBuilder useDateAsTimestamp(boolean enabled) {
    this.dateTimeConfigModifier.andThen(config -> config.setDateAsTimestamp(enabled));
    return this;
}

We don't have to worry about nulls, we don't have to worry about merging, we don't have to worry about ordering -- all we have to do is apply the modifiers, in the order they were defined both by the builder methods and any configured bundles, simply by doing the following in the create method:

DateTimeConfig config = dateTimeConfigModifier.apply(new DateTimeConfig());

// or, if we want config to be truly immutable and we have copy constructor on the config object
DateTimeConfig config = new DateTimeConfig(dateTimeConfigModifier.apply(new DateTimeConfig()));

Hope you agree that the benefits far outweigh the downsides. I'd also like to hear what @nicky9door thinks, as he's the one who needs to make the changes to this PR based on all this ;-)

[aseovic]

BTW, I've used Function<DTC, DTC> above to represent the config modifier (Consumer<DTC> is wrong, as we do need it returned), but in reality we should probably:

1. Define our own functional interface in order to make the API cleaner and simpler:

@FunctionalInterface
public interface Modifier<T> {
    T apply(T obj);
    
   default Modifier<T> andThen(Modifier<T> after) {
        Objects.requireNonNull(after);
        return t -> after.apply(apply(t));
    }

    static <T> Modifier<T> identity() {
        return t -> t;
    }  
}

2. Define copy constructor and factory method on config objects

public class DateTimeConfig {
    private DateTimeConfig(DateTimeConfig from) {
        // copy ctor
    }

    public static create(Modifier<DateTimeConfig> modifier) {
        return new DateTimeConfig(modifier.apply(new DateTimeConfig())); 
    }
}

3. Use the above within GensonBuilder.create method

DateTimeConfig dtc = DateTimeConfig.create(dateTimeConfigModifier);

That way we can a) encapsulate creation logic within config objects and ensure immutability; and b) make it really easy to create various config objects within the builder.

[rlubke]

This will be a great addition to Genson. I've been testing this with our product and found that Converter implementations for Duration, ZoneId, and ZoneOffset would be desirable.