Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use RFC 3339 format for timestamp #669

Merged
merged 9 commits into from
Nov 28, 2017
Merged

Use RFC 3339 format for timestamp #669

merged 9 commits into from
Nov 28, 2017

Conversation

corinaminer
Copy link
Contributor

@corinaminer corinaminer commented Nov 15, 2017

Updated timestamp to use RFC 3339's accepted timestamp format (see RFC 3339 sections 5.6, 5.8; example: 1985-04-12T23:20:50.52Z). Also renamed the timestamp property in the metadata to creationTimestamp.


This change is Reviewable

@dhalperi
Copy link
Member

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgr.java, line 800 at r1 (raw file):

    // Create metadata file (RELPATH_METADATA_FILE is "metadata.json")
    Path metadataPath = testrigDir.resolve(BfConsts.RELPATH_METADATA_FILE);
    CommonUtil.writeFile(

Here's my general tips for making this more future-proof: a) don't construct JSON strings manually and b) let the mapping libraries to the serialization work in a general way. Here's what I'd do:

  1. Please create a class TestrigMetadata intended for use in Jackson serialization/deserialization. I guess right now it just has a creationTimestamp field of time java.time.Instant

  2. Update BatfishObjectMapper (henceforth, "BOM") to disable serializing date-types as timestamps: disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS);

  3. Add a test of BOM that verifies that some fixed date serializes in the RFC format (say your birthday, I don't care as long as all the fields are interesting).

  4. Update this to use BOM.writeValueAsString on the metadata class.


Comments from Reviewable

@dhalperi
Copy link
Member

projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgr.java, line 800 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Here's my general tips for making this more future-proof: a) don't construct JSON strings manually and b) let the mapping libraries to the serialization work in a general way. Here's what I'd do:

  1. Please create a class TestrigMetadata intended for use in Jackson serialization/deserialization. I guess right now it just has a creationTimestamp field of time java.time.Instant

  2. Update BatfishObjectMapper (henceforth, "BOM") to disable serializing date-types as timestamps: disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS);

  3. Add a test of BOM that verifies that some fixed date serializes in the RFC format (say your birthday, I don't care as long as all the fields are interesting).

  4. Update this to use BOM.writeValueAsString on the metadata class.

May also need to add and/or register the Java8 jackson library, which adds knowledge to Jackson that java.time.Instant (new in Java8) is a date-like class. https://github.com/FasterXML/jackson-modules-java8/tree/master/datetime


Comments from Reviewable

@dhalperi
Copy link
Member

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@dhalperi
Copy link
Member

Reviewed 6 of 6 files at r2.
Review status: 4 of 6 files reviewed at latest revision, 3 unresolved discussions.


projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/TestrigMetadata.java, line 8 at r2 (raw file):

public class TestrigMetadata {
  private static final String PROP_CREATIONTIMESTAMP = "creationTimestamp";
  private java.time.Instant _creationTimestamp;

just import java.time.Instant; and use Instant here. Unless there's some reason not to that I'm missing.


projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/TestrigMetadataTest.java, line 22 at r2 (raw file):

          equalTo("{\n  \"creationTimestamp\" : \"1994-01-19T03:10:05.001Z\"\n}"));
    } catch (Exception e) {
      assertThat(e.getMessage(), 0, equalTo(1));

just add throws Exception to the function definition and drop the try/catch.


projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgr.java, line 801 at r2 (raw file):

    // Create metadata file (RELPATH_METADATA_FILE is "metadata.json")
    BatfishObjectMapper mapper = new BatfishObjectMapper();
    TestrigMetadata metadata = new TestrigMetadata(java.time.Instant.now());

same comment about Instant here.


Comments from Reviewable

@dhalperi
Copy link
Member

Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@corinaminer
Copy link
Contributor Author

Review status: 2 of 6 files reviewed at latest revision, 3 unresolved discussions.


projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/TestrigMetadata.java, line 8 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

just import java.time.Instant; and use Instant here. Unless there's some reason not to that I'm missing.

Done.


projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/TestrigMetadataTest.java, line 22 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

just add throws Exception to the function definition and drop the try/catch.

Done.


projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgr.java, line 801 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

same comment about Instant here.

Done.


Comments from Reviewable

@dhalperi
Copy link
Member

:lgtm:


Reviewed 4 of 4 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@dhalperi dhalperi merged commit 75011fc into master Nov 28, 2017
@dhalperi dhalperi deleted the rfc3339-timestamp branch November 28, 2017 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants