Skip to content

Commit

Permalink
Issue/FasterXML#116 (#4)
Browse files Browse the repository at this point in the history 
* test: add initial test case

* fix: fix problems in test case

for issue #1

* feat: add test cases for timestamps part and nanoseconds part

related to issue #1

* create a false test cases

#1

* Use flag for diabling fraction

* Use flag for diabling fraction

* Added the flag for DurationSerializer classes

* Removed test cases that are not relevant to the added Serialization Feature

* Only keep requirement specific tests

Some tests were set up to get an understanding of the code. They should
not be kept and included in the pull request

Co-authored-by: Nagisa <[email protected]>
Co-authored-by: Jacob Adlers <[email protected]>
  • Loading branch information
@sashikanthr @ChangFu2000 @jadlers
3 people authored Feb 27, 2020
1 parent ac5aec3 commit e837c02
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 3 deletions.
4 changes: 3 additions & 1 deletion datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/ser/DurationSerializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,9 @@ public void serialize(Duration duration, JsonGenerator generator,
SerializerProvider provider) throws IOException
{
if (useTimestamp(provider)) {
if (useNanoseconds(provider)) {
if (withoutFraction(provider)) {

This comment has been minimized.

Sign in to view

Copy link
@kupci

kupci Mar 1, 2020

how about useEpochSeconds instead of withoutFraction, to be a little more consistent with existing code/naming conventions?

This comment has been minimized.

Sign in to view

Copy link
@jadlers

jadlers Mar 2, 2020

Author Collaborator

Yes, that sounds a lot better!
generator.writeNumber(duration.toMillis()/1000);
} else if (useNanoseconds(provider)) {
generator.writeNumber(DecimalUtils.toBigDecimal(
duration.getSeconds(), duration.getNano()
));
Expand Down
7 changes: 5 additions & 2 deletions datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/ser/InstantSerializerBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,16 @@ protected abstract JSR310FormattedSerializerBase<?> withFormat(DateTimeFormatter
public void serialize(T value, JsonGenerator generator, SerializerProvider provider) throws IOException
{
if (useTimestamp(provider)) {
if (useNanoseconds(provider)) {
if (withoutFraction(provider)) {
generator.writeNumber(getEpochMillis.applyAsLong(value) / 1000);

This comment has been minimized.

Sign in to view

Copy link
@kupci

kupci Mar 1, 2020

You should be able to avoid the divide if instead of calling the getEpochMillis, call
generator.writeNumber(getEpochSeconds.applyAsLong(value));

This comment has been minimized.

Sign in to view

Copy link
@kupci

kupci Mar 1, 2020

Also, it would seem a little safer to add this new if-block after the original if (useNanoseconds( just in case some folks make a mistake somehow and have both flags set to true, now this new code will override the original flag

This comment has been minimized.

Sign in to view

Copy link
@jadlers

jadlers Mar 2, 2020

Author Collaborator

I agree with the removal of the division.

As for changing the order of the if block there is a problem with WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS being true by default. Having both feature flags, WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS and WRITE_TIMESTAMPS_WITHOUT_FRACTION (where the latter is updated to something like WRITE_DATE_TIMESTAMPS_AS_SECONDS), would not make sense. Both cannot be applied.

Do you think this feature should explicitly require the user to disable WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS as well as enabling WRITE_DATE_TIMESTAMPS_AS_SECONDS?

This comment has been minimized.

Sign in to view

Copy link
@jadlers

jadlers Mar 2, 2020

Author Collaborator

The behaviour is also documented in the PR we created in FasterXML/jackson-databind which can be seen here

This comment has been minimized.

Sign in to view

Copy link
@kupci

kupci Mar 2, 2020

(Caveat that given CowTownCoder's comments on the PR, he might want to go a different route, whether using DateTimeConfig, or the Shape.NUMBER_FLOAT, but I'll answer the questions here anyway as it's helpful to discuss I think and refine the changes.)

WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS is false by default, or, at least, it should be. and then the new one, let's call it WRITE_DATE_TIMESTAMPS_AS_EPOCHSECONDS, is also defaulting to false.

Having both feature flags, WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS and WRITE_TIMESTAMPS_WITHOUT_FRACTION (where the latter is updated to something like WRITE_DATE_TIMESTAMPS_AS_SECONDS), would not make sense. Both cannot be applied.

Ay, there's the rub, as Hamlet would say. The reason I was thinking of putting this logic after the nanos check is to preserve the functionality, for any user who unwittingly (somehow) sets both flags to true. This way, this reduces the possibility of someone complaining on the issues list about how something used to work, and now it doesn't (given this is unlikely, since they'd actually have to explicitly set the flag.

Do you think this feature should explicitly require the user to disable WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS as well as enabling WRITE_DATE_TIMESTAMPS_AS_[EPOCH]SECONDS?

That's a thought, an exception could be thrown in that case. And that way the ordering wouldn't matter, as there could only possibly be one path.

The behaviour is also documented in the PR we created

I'll add a note to the PR, but I think (should this PR go through), there should be a cross-reference in WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS.

This comment has been minimized.

Sign in to view

Copy link
@kupci

kupci Mar 2, 2020 •
edited
Loading

Ok, on the defaulting, I stand corrected, looks like the README is out of date, assuming the java doc is correct

     *<p>
     * If disabled, standard millisecond timestamps are assumed.
     * This is the counterpart to {@link DeserializationFeature#READ_DATE_TIMESTAMPS_AS_NANOSECONDS}.
     *<p>
     * Feature is enabled by default, to support most accurate time values possible.

Which is contrary to what CowTownCoder mentioned in the comments to this PR - we should be defaulting to milliseconds. Hmm. Not sure how that slipped through.
return;
} else if (useNanoseconds(provider)) {
generator.writeNumber(DecimalUtils.toBigDecimal(
getEpochSeconds.applyAsLong(value), getNanoseconds.applyAsInt(value)
));
return;
}
generator.writeNumber(getEpochMillis.applyAsLong(value));
generator.writeNumber(getEpochMillis.applyAsLong(value));

This comment has been minimized.

Sign in to view

Copy link
@kupci

kupci Mar 1, 2020

formatting is off?

This comment has been minimized.

Sign in to view

Copy link
@jadlers

jadlers Mar 2, 2020

Author Collaborator

We did not find any documentation about formatting to use when contributing. How should we format the code?

This comment has been minimized.

Sign in to view

Copy link
@kupci

kupci Mar 2, 2020

Ok, could be a github glitch, I've seen that before. As long as it looks correct in the code itself.
return;
}
String str;
Expand Down
5 changes: 5 additions & 0 deletions ...rc/main/java/com/fasterxml/jackson/datatype/jsr310/ser/JSR310FormattedSerializerBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,4 +240,9 @@ protected boolean useNanoseconds(SerializerProvider provider) {
return (provider != null)
&& provider.isEnabled(SerializationFeature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS);
}

protected boolean withoutFraction(SerializerProvider provider) {
return (provider != null)
&& provider.isEnabled(SerializationFeature.WRITE_TIMESTAMPS_WITHOUT_FRACTION);
}
}
20 changes: 20 additions & 0 deletions datetime/src/test/java/com/fasterxml/jackson/datatype/jsr310/ser/DurationSerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,24 @@ public void testSerializationWithTypeInfo03() throws Exception
assertEquals("The value is not correct.",
"[\"" + Duration.class.getName() + "\",\"" + duration.toString() + "\"]", value);
}

/**
* Tests the Serialization Feature 'WRITE_TIMESTAMPS_WITHOUT_FRACTION' for Duration Serializer.
* @throws Exception if there is an issue while processing the JSON
*/
@Test
public void testDurationSerializationWithFractionFeature() throws Exception
{
ObjectMapper mapper = newMapperBuilder().enable(SerializationFeature.WRITE_DURATIONS_AS_TIMESTAMPS)
.disable(SerializationFeature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS)
.enable(SerializationFeature.WRITE_TIMESTAMPS_WITHOUT_FRACTION)
.addMixIn(TemporalAmount.class, MockObjectConfiguration.class)
.build();
Duration duration = Duration.ofSeconds(13498L, 8374);
String value = mapper.writeValueAsString(duration);

assertNotNull("The value should not be null.", value);
assertEquals("The value is not correct.",
"[\"" + Duration.class.getName() + "\"," + duration.getSeconds() + "]", value);
}
}
57 changes: 57 additions & 0 deletions datetime/src/test/java/com/fasterxml/jackson/datatype/jsr310/ser/InstantSerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

package com.fasterxml.jackson.datatype.jsr310.ser;

import com.fasterxml.jackson.annotation.JsonFormat;
import com.fasterxml.jackson.annotation.JsonGetter;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializationFeature;
import com.fasterxml.jackson.datatype.jsr310.DecimalUtils;
Expand Down Expand Up @@ -173,4 +176,58 @@ public void testSerializationWithTypeInfo03() throws Exception
assertEquals("The value is not correct.",
"[\"" + Instant.class.getName() + "\",\"" + FORMATTER.format(date) + "\"]", value);
}

/**
* When {@link
* com.fasterxml.jackson.code.jackson-databind.WRITE_TIMESTAMPS_WITHOUT_FRACTION}
* is enabled no fraction is added to the serialized string.
*
* @throws Exception
*/
@Test
public void testSerializationWithFractionFlag() throws Exception
{
class TempClass {
@JsonProperty("registered_at")
@JsonFormat(shape = JsonFormat.Shape.NUMBER)
private Instant registeredAt;

public TempClass(long seconds, int nanos) {
this.registeredAt = Instant.ofEpochSecond(seconds, nanos);
}
}

String value = MAPPER.writer()
.with(SerializationFeature.WRITE_TIMESTAMPS_WITHOUT_FRACTION)
.writeValueAsString(new TempClass(1420324047L, 123456));
assertEquals("The value should not include any decimals.", "{\"registered_at\":1420324047}", value);
}

/**
* When both WRITE_TIMESTAMPS_WITHOUT_FRACTION and
* WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS from {@link
* com.fasterxml.jackson.code.jackson-databind} are enabled the former one

This comment has been minimized.

Sign in to view

Copy link
@kupci

kupci Mar 1, 2020

Good check, and I was wondering about this possible scenario. I'd assume, given this is newly added, to defer to the original code and give that precedence, in case someone makes a mistake.
* has priority.
*
* @throws Exception
*/
@Test
public void testSerializationFractionPriority() throws Exception
{
class TempClass {
@JsonProperty("registered_at")
@JsonFormat(shape = JsonFormat.Shape.NUMBER)
private Instant registeredAt;

public TempClass(long seconds, int nanos) {
this.registeredAt = Instant.ofEpochSecond(seconds, nanos);
}
}

String value = MAPPER.writer()
.with(SerializationFeature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS)
.with(SerializationFeature.WRITE_TIMESTAMPS_WITHOUT_FRACTION)
.writeValueAsString(new TempClass(1420324047L, 123456));
assertEquals("The value does not include any decimals.", "{\"registered_at\":1420324047}", value);
}
}

0 comments on commit e837c02

Please sign in to comment.