Add Instant support. #125
Add Instant support. #125
Conversation
| Object enabled = configurationValues.get("hibernate.type.java_time_use_direct_jdbc"); | ||
| if (enabled instanceof Boolean && (Boolean) enabled) { | ||
| throw new HibernateException(format( | ||
| "Configuration property [%s] is incubating and not supported in MongoDB dialect", |
There was a problem hiding this comment.
Offline review with Valentin:
Remove "is incubating" and "Mongo dialect"
| checkClosed(); | ||
| checkParameterIndex(parameterIndex); | ||
| throw new SQLFeatureNotSupportedException("TODO-HIBERNATE-42 https://jira.mongodb.org/browse/HIBERNATE-42"); | ||
| if (!"UTC".equals(calendar.getTimeZone().getID())) { |
There was a problem hiding this comment.
Offline review with Valentin:
Change to "calendarZoneId.getRules().equals(ETC_UTC_ZONE_RULES)"
HIBERNATE-42
vbabanin
force-pushed
the
HIBERNATE-42-instant
branch
from
ac167e0 to
d7b51e5
Compare
...ava/com/mongodb/hibernate/internal/extension/service/StandardServiceRegistryScopedState.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public TimeZoneSupport getTimeZoneSupport() { | ||
| return TimeZoneSupport.NONE; |
There was a problem hiding this comment.
Turns out the default implementation is the same. Do we need the override at all?
The unnecessary override is removed in stIncMale@a7d9ae1.
|
|
||
| @Override | ||
| public void appendDatetimeFormat(final SqlAppender appender, final String format) { | ||
| throw new FeatureNotSupportedException("TODO-HIBERNATE-88 https://jira.mongodb.org/browse/HIBERNATE-88"); |
There was a problem hiding this comment.
When we add a comment like this, we also mention
Addressing the source code notes tagged with TODO-HIBERNATE-88 is in scope of this ticket.
in the description of the ticket, to remind the assignee to look for such TODOs. I have added note to the description.
| @Override | ||
| public TimeZoneSupport getTimeZoneSupport() { | ||
| return TimeZoneSupport.NONE; | ||
| } | ||
|
|
||
| @Override | ||
| public void appendDatetimeFormat(final SqlAppender appender, final String format) { | ||
| throw new FeatureNotSupportedException("TODO-HIBERNATE-88 https://jira.mongodb.org/browse/HIBERNATE-88"); | ||
| } |
There was a problem hiding this comment.
This is a weird placement of the methods: after one of the MongoDialect constructors, and before the other one. I propose adding these methods to the end of the class.
The proposed change is done in stIncMale@19afd7f.
| return Stream.of( | ||
| Arguments.of(TimeZone.getTimeZone("UTC"), TimeZone.getTimeZone("GMT+1")), | ||
| Arguments.of(TimeZone.getTimeZone("GMT+1"), TimeZone.getTimeZone("GMT+1")), | ||
| Arguments.of(TimeZone.getTimeZone("GMT+1"), TimeZone.getTimeZone("GMT+2")), | ||
| Arguments.of(TimeZone.getTimeZone("GMT+05:31"), TimeZone.getTimeZone("GMT+05:45")), | ||
| Arguments.of(TimeZone.getTimeZone("America/New_York"), TimeZone.getTimeZone("America/Los_Angeles"))); |
There was a problem hiding this comment.
Running InstantIntegrationTests takes 17 seconds, with the whole test set barring InstantIntegrationTests taking less than 1 minute. Let's reduce the number of variants, unless they seem very useful.
To me it seems that the only pairs worth checking are when the timezones differ (maybe in both directions), regardless of by how much.
Also, the canonical variants of the UTC and GMT+.../GMT-... timezone IDs are prefixed with Etc/ (Etc is short for "et cetera", as timezone IDs should follow the {area}/{city} form): see https://en.wikipedia.org/wiki/List_of_tz_database_time_zones (unfortunately, there is no authoritative web view of the time zone database).
The proposed change is done in stIncMale@baa1594.
| @ExtendWith(MongoExtension.class) | ||
| class InstantIntegrationTests implements SessionFactoryScopeAware { | ||
|
|
||
| private static final TimeZone CURRENT_JVM_TIMEZONE = TimeZone.getDefault(); |
There was a problem hiding this comment.
Let's rename current->original, because as soon as we change the default time zone, the "current" one stops being the current.
The suggestion is done in stIncMale@bdaf0b2.
| return Stream.of( | ||
| Arguments.of( | ||
| systemDefaultTimeZone, | ||
| jdbcTimezone, | ||
| // Attribute or an element of an attribute to save. | ||
| Instant.parse("2007-12-03T10:15:30.00Z"), | ||
| // Expected attribute or an element of an attribute after read. | ||
| Instant.parse("2007-12-03T10:15:30.00Z")), | ||
| Arguments.of( | ||
| systemDefaultTimeZone, | ||
| jdbcTimezone, | ||
| Instant.parse("1500-12-03T10:15:30Z"), | ||
| Instant.parse("1500-12-03T10:15:30Z")), | ||
| Arguments.of( | ||
| systemDefaultTimeZone, | ||
| jdbcTimezone, | ||
| Instant.parse("-000001-12-03T10:15:30Z"), | ||
| Instant.parse("-000001-12-03T10:15:30Z")), | ||
| Arguments.of( | ||
| systemDefaultTimeZone, | ||
| jdbcTimezone, | ||
| // Other dialects might support nanoseconds precision, however, in our case the precision is | ||
| // to within milliseconds. | ||
| Instant.parse("2007-12-03T10:15:30.000000999Z"), | ||
| Instant.parse("2007-12-03T10:15:30Z")), | ||
| Arguments.of( | ||
| systemDefaultTimeZone, | ||
| jdbcTimezone, | ||
| // We support milliseconds precision, so nanoseconds are rounded down to milliseconds. | ||
| Instant.parse("2007-12-03T10:15:30.002900000Z"), | ||
| Instant.parse("2007-12-03T10:15:30.002000000Z"))); |
There was a problem hiding this comment.
The 4th of the Arguments does not seem to add anything to the 5th (last) one in terms of what is being tested. Given that running InstantIntegrationTests takes quite a lot of time (17 seconds, with the whole test set barring InstantIntegrationTests taking less than 1 minute), let's get rid of the 4th Arguments.
This proposed change is done in stIncMale@4e72f84.
Further, the 1st one does not seem to add anything to the 5th one, so let's remove it also.
This proposed change is done in stIncMale@d81ffa1.
| }); | ||
| } | ||
|
|
||
| /** Write: system tz: T1, session tz: T2 Read: system tz: T1, session tz: T2 */ |
There was a problem hiding this comment.
To be readable, this comment needs punctuation between the sentences. The same applies to other similar comments.
The proposed change is done in stIncMale@08eaa77.
| @ParameterizedTest( | ||
| name = "Instant: system TZ differ per read/write; session TZ equals system TZ;" | ||
| + "systemDefaultTimeZone={0}, jdbcTimeZone={1}") |
There was a problem hiding this comment.
The name of the testInstantRoundTripWhenSessionTzEqual test says
system TZ not equal session TZ
But the name here says
session TZ equals system TZ
It would have been helpful if those parts were mentioning time zones consistently. For example, at first the system TZ, then the session TZ.
On one hand, the name is needed here to make the test cases distinguishable in Evergreen. On the other hand, it duplicates the information specified in the test method documentation comment. Given that names are unavoidable, let's make them such that comments are not needed.
The testInstantRoundTripWhenSessionTzEqual, testInstantRoundTripWhenSessionTzNotEqual, testInstantPersistAndReadDifferentTimeZones test method names are hardly meaningful. Given that the name is used and is supposed to describe the meaning, let's simplify the test method names.
- The
Instant:prefix in thenames is not adding any information, because the class name is alreadyInstantIntegrationTests.
- The arguments of the test methods should be of the
ZoneIdtype, notTimeZone, because theTimeZone.toStringemits rules, which makes thenameunreadable:com.mongodb.hibernate.InstantIntegrationTests.Instant__system_TZ_equal_per_read_write__system_TZ_not_equal_session_TZ__session_TZ_equal_per_read_write_systemDefaultTimeZone_sun.util.calendar.ZoneInfo_id__UTC__offset_0_dstSavings_0_useDaylight_false_transitions_0_lastRule_null___jdbcTimeZone_sun.util.calendar.ZoneInfo_id__UTC__offset_0_dstSavings_0_useDaylight_false_transitions_0_lastRule_null_
The proposed changes are done in stIncMale@c10a702.
There was a problem hiding this comment.
1 - Made consistent as proposed.
2 - In the referenced commit, all parameterized tests render the same Write: system tz {n}, session tz {n}. Read: system tz {n}, session tz {n} pattern.
When multiple tests fail at once, it can become tricker to identify which configuration failed without cross-referencing parameters or examining the log for the method name.
I agree we can move explanatory comments into method names. We could also prefix each test name with its invariants. This would keep the Write(sys={0}, sess={1}). Read(sys={0}, sess={1}) format while making failures distinguishable in Evergreen.
Example
sys TZ equal per write/read; sys TZ not equal sess TZ; sess TZ equal per write/read.
Write(sys={0}, sess={1}). Read(sys={0}, sess={1})
Updated to:
methodName: Write(sys={0}, sess={1}). Read(sys={0}, sess={1})
3 - Scenario-based method names are easier to locate in the IDE when browsing by methods, since the @ParameterizedTest display name is not shown there. I suggest we keep short semantic method names that summarize each scenario (I’ve shortened them accordingly).
4 - Removed the redundant "Instant:" prefix.
5 - Adopted ZoneId instead of TimeZone.
Let me know what you think.
There was a problem hiding this comment.
TODO: use method name instead of lengthy test name (which evergreen substitutes it with _ ; hard to copy)
UPD: done
| Collection<StructAggregateEmbeddableIntegrationTests.Single> structAggregateEmbeddablesCollection) { | ||
| Collection<StructAggregateEmbeddableIntegrationTests.Single> structAggregateEmbeddablesCollection, | ||
| Instant[] instants, | ||
| List<Instant> instantsCollection) { |
There was a problem hiding this comment.
For the sake of writing code that follows the existing code structure and style:
- The
Instant[] instantsparameter should be positioned immediately after theObjectId[] objectIdsparameter. - The
List<Instant> instantsCollectionparameter should be positioned immediately after theCollection<ObjectId> objectIdsCollectionparameter. - The type of the
instantsCollectionparameter should beCollection<Instant>. - The
this.instants = instantsinitialization should be placed immediately afterthis.objectIds = objectIds. StructAggregateEmbeddableIntegrationTests.ArraysAndCollections,ArrayAndCollectionIntegrationTests.ItemWithArrayAndCollectionValueshave similar, though not exactly the same issues.- The field order in the expected documents should be adjusted to match the previous changes for readability.
- When creating
ArraysAndCollectionsinEmbeddableIntegrationTests.testFlattenedValueHavingArraysAndCollections/StructAggregateEmbeddableIntegrationTests.testNestedValueHavingArraysAndCollections, thenullelement should be added to arrays and collections.
The proposed changes are done in stIncMale@9b7cc21.
|
|
||
| @Entity | ||
| @Table(name = COLLECTION_NAME) | ||
| @Table(name = "items") |
There was a problem hiding this comment.
This change is not needed, and should be reverted.
The proposed change is done in stIncMale@579e32d.
| ItemDynamicallyUpdated() {} | ||
|
|
||
| ItemDynamicallyUpdated(int id, boolean primitiveBoolean, Boolean boxedBoolean) { | ||
| ItemDynamicallyUpdated(int id, boolean primitiveBoolean, Boolean boxedBoolean, Instant instant) { |
There was a problem hiding this comment.
No changes to ItemDynamicallyUpdated is needed here. This entity is not supposed to test all the basic types.
The proposed change is done in stIncMale@0765459.
There was a problem hiding this comment.
I skipped reviewing the changes to this file, in hope that the alternative design I propose (use setObject/getObject for Instant) will be accepted. If it is, we'll just revert the changes to this file. If not, I'll have to review it.
TODO for @stIncMale: review what was skipped, if still needed.
|
|
||
| abstract List<I> getData(); | ||
|
|
||
| @ParameterizedTest(name = "testComparisonByEq: temporal={0}, expectedItems={2}, expectedRender={3}, timeZone={1}") |
There was a problem hiding this comment.
There is no timeZone parameter here, or in any other test methods declared in this class, which suggests that something is wrong with the file.
I skipped reviewing AbstractSelectionTemporalIntegrationTest and SelectionInstantIntegrationTest. I am asking to please at first self-review them, taking into account the rest of the comments in this PR, and do the changes, if needed.
TODO for @stIncMale: review what was skipped.
| checkParameterIndex(parameterIndex); | ||
| throw new SQLFeatureNotSupportedException("TODO-HIBERNATE-42 https://jira.mongodb.org/browse/HIBERNATE-42"); | ||
| checkTimeZone(calendar.getTimeZone()); | ||
| setParameter(parameterIndex, toBsonValue(timestamp)); |
There was a problem hiding this comment.
There is a way to make Hibernate ORM to use setObject/getObject for Instant, thus saving us from dealing with Timestamp and Calendar, without dealing with the JAVA_TIME_USE_DIRECT_JDBC, PREFERRED_INSTANT_JDBC_TYPE configuration properties. I propose us to use it, as it makes the implementation and even testing simpler.
The proposed change is done in stIncMale@5bebe91.
|
The PR with all the proposed changes: vbabanin#1. |
# Conflicts: # src/main/java/com/mongodb/hibernate/dialect/MongoDialect.java
- Add tests for the forbidden temporal types. - Use TimestampUtcAsInstantJdbcType. - Rename test method and display names. HIBERNATE-42
Add test case for flattened unsupported id types.
2 participants
Hibernate ORM currently handles temporal types (Instant, etc.) inconsistently depending on where they appear in an entity. For example, saving an Instant of 10:15 (always UTC) on a host in GMT+1 leads to inconsistent results once read (tested with Postgres dialect):
This inconsistency makes it difficult to predict how temporal values will be stored and retrieved.
This PR ensures that temporal types are handled consistently in MongoDB extension across top-level fields, arrays/collections, and structs, so that serialization and deserialization behave uniformly regardless of context.
HIBERNATE-42