Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Implemented
    • Affects Version/s: 3.1M2
    • Fix Version/s: 4.0.M3
    • Component/s: Database integration
    • Labels:
      None

      Description

      Here is some code that we wrote at Swrve to load/save jodatime DateTime objects with Cayenne

      1. 0001-Add-support-for-joda-time-attributes.patch
        19 kB
        John Huss
      2. 0002-Add-support-for-joda-time-attributes.patch
        31 kB
        John Huss
      3. 0003-CAY-1626-Add-JodaTime-DateTime-support.patch
        30 kB
        John Huss
      4. DateTimeType.java
        3 kB
        Marc O'Morain
      5. joda-patch.txt
        18 kB
        John Huss

        Activity

        Hide
        John Huss added a comment -

        The attached file has some issues:

        • The string format of the timestamp is database specific so it can't be relied upon. For example with Postgres this doesn't work. To avoid problems with the driver changing the time zone you should modify the driver or just change the default time zone of your app to use GMT.
        • Using a string with this format limits the precision of the time value since milliseconds are only given to 1 decimal place.

        I'm interested in the Joda time library myself, so I took a stab at doing this and also added support for some of the other useful joda classes.
        So my patch has support for: DateTime, LocalDate, and LocalTime. It also includes the custom serializers necessary for using these classes in an ROP app. The patch has new classes only. The new extended types and serializers aren't registered anywhere currently, so the user has to do that manually.

        Show
        John Huss added a comment - The attached file has some issues: The string format of the timestamp is database specific so it can't be relied upon. For example with Postgres this doesn't work. To avoid problems with the driver changing the time zone you should modify the driver or just change the default time zone of your app to use GMT. Using a string with this format limits the precision of the time value since milliseconds are only given to 1 decimal place. I'm interested in the Joda time library myself, so I took a stab at doing this and also added support for some of the other useful joda classes. So my patch has support for: DateTime, LocalDate, and LocalTime. It also includes the custom serializers necessary for using these classes in an ROP app. The patch has new classes only. The new extended types and serializers aren't registered anywhere currently, so the user has to do that manually.
        Hide
        John Huss added a comment -

        Updated patch with some bug fixes

        Show
        John Huss added a comment - Updated patch with some bug fixes
        Hide
        John Huss added a comment -

        Patch to add joda support now prepped for inclusion - please review.

        Show
        John Huss added a comment - Patch to add joda support now prepped for inclusion - please review.
        Hide
        Andrus Adamchik added a comment -

        Looks pretty good. I like the idea to a use a Module to ensure it is entirely optional. A few notes:

        1. Joda dependency scope should probably be "provided" at framework/cayenne-core-unpublished/pom.xml. Doesn't matter much probablyas it is repackaged into cayenne-server eventually, but just to underscore that this is an extension.

        2. Let's use SERVER_DEFAULT_TYPES_LIST instead of SERVER_USER_TYPES_LIST in the JodaModule - after all these are framework classes.

        3. Maybe we can improve on 'isJodaAvailable' check in CommonsJdbcEventLogger ? Wonder if we can rewrite the whole 'sqlLiteralForObject' method and instead of guessing the print format of each object type, we take it from say ExtendedType. Extended types can be injected, like we are injecting them to DbAdapters.

        Show
        Andrus Adamchik added a comment - Looks pretty good. I like the idea to a use a Module to ensure it is entirely optional. A few notes: 1. Joda dependency scope should probably be "provided" at framework/cayenne-core-unpublished/pom.xml. Doesn't matter much probablyas it is repackaged into cayenne-server eventually, but just to underscore that this is an extension. 2. Let's use SERVER_DEFAULT_TYPES_LIST instead of SERVER_USER_TYPES_LIST in the JodaModule - after all these are framework classes. 3. Maybe we can improve on 'isJodaAvailable' check in CommonsJdbcEventLogger ? Wonder if we can rewrite the whole 'sqlLiteralForObject' method and instead of guessing the print format of each object type, we take it from say ExtendedType. Extended types can be injected, like we are injecting them to DbAdapters.
        Hide
        Andrus Adamchik added a comment -

        4. Also we probably need at least basic unit tests

        Show
        Andrus Adamchik added a comment - 4. Also we probably need at least basic unit tests
        Hide
        John Huss added a comment -

        Good thoughts. Regarding point 3 (rewriting sqlLiteralForObject) we can break up the existing switch statement by adding a new method to ExtendedType that each type can override. But the strings generated by those methods are still basically guesswork, although it doesn't matter much because these are only used for writing the sql debug log.

        Show
        John Huss added a comment - Good thoughts. Regarding point 3 (rewriting sqlLiteralForObject) we can break up the existing switch statement by adding a new method to ExtendedType that each type can override. But the strings generated by those methods are still basically guesswork, although it doesn't matter much because these are only used for writing the sql debug log.
        Hide
        John Huss added a comment -

        Updated patch following feedback. I haven't made any changes to sqlLiteralForObject. If we want to do that, I think it should go in a separate commit / task.

        Show
        John Huss added a comment - Updated patch following feedback. I haven't made any changes to sqlLiteralForObject. If we want to do that, I think it should go in a separate commit / task.
        Hide
        John Huss added a comment -

        Updated patch. No code changes, just re-integrated with current trunk. This is ready for review again.

        Show
        John Huss added a comment - Updated patch. No code changes, just re-integrated with current trunk. This is ready for review again.
        Hide
        John Huss added a comment -

        This has been outstanding for a long time. Any objections to merging it after rebasing it on master?

        Since Java 8 support is still a ways out and many people (me) have existing code using joda-time this is still a useful feature.

        Show
        John Huss added a comment - This has been outstanding for a long time. Any objections to merging it after rebasing it on master? Since Java 8 support is still a ways out and many people (me) have existing code using joda-time this is still a useful feature.

          People

          • Assignee:
            Savva Kolbachev
            Reporter:
            Marc O'Morain
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development