Fixed JSON annotations to enable ODE processing of circle geometries #114

dmccoystephenson · 2024-11-04T21:27:58Z

PR Details

Description

Problem

When preparing a TIM for encoding by translating it to XML, the position element is mistakenly added to the circle element. This occurs because a JSON property annotation incorrectly assigns position to the getter for the center element of circle.

Solution

The issue was resolved by updating the Circle subclass of Asn1Object to use consistent JSON property annotations for the center element.

Related Issue

Motivation and Context

As per J2735, the ODE should be capable of processing circle geometries for TIMs.

How Has This Been Tested?

A TIM request with a circle geometry of the following form was sent to the /tim endpoint:

"geometry": {
  "direction": "1111000011110000",
  "circle": {
    "center": {
      "latitude": "41.153593",
      "longitude": "-104.655785",
      "elevation": "1824"
    },
    "radius": "50",
    "units": "3"
  }
},

Given this input, the ODE produced input XER for the AEM that it was able to process.

A new unit test has been added to verify that the TimTransmogrifier correctly converts a TIM with circle geometry to XML. The test prepares input data similarly to the TimDepositController, though the setup in the unit test is slightly simplified. While further refactoring of the data preparation process would be beneficial, it is outside the scope of this fix.

It is recommended to set the 'show whitespace' option to false when reviewing the TimTransmogrifierTest file.

Types of changes

Defect fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that cause existing functionality to change)

Checklist:

I have added any new packages to the sonar-scanner.properties file
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
ODE Contributing Guide
I have added tests to cover my changes.
All new and existing tests passed.

…1Object

…ject

dmccoystephenson · 2024-11-04T21:31:19Z

jpo-ode-plugins/src/main/java/us/dot/its/jpo/ode/plugin/j2735/timstorage/Circle.java


-   @JsonProperty("position")


note: this is the offending annotation that caused the problem

Can we add a unit test for this use case?

Is position an invalid property on a Circle Asn1Object in all cases, or is there any scenario where we want to have position present? I'm gathering that, at minimum, we don't want both position and center present, but is there a scenario where we could want position present when center is not (and vice versa)?

Is position an invalid property on a Circle Asn1Object in all cases, or is there any scenario where we want to have position present? I'm gathering that, at minimum, we don't want both position and center present, but is there a scenario where we could want position present when center is not (and vice versa)?

The ASN.1 representation for a Circle in J2735 only contains center, radius and units. I don't think we'll ever have a scenario where we want to have 'position' present.

Michael7371

I was able to verify that circle TIM's process properly and that unit tests all pass. My only comment is that we should add a unit test to cover this bug fix.

Michael7371 · 2024-11-04T23:02:11Z

jpo-ode-plugins/src/main/java/us/dot/its/jpo/ode/plugin/j2735/timstorage/Circle.java


-   @JsonProperty("position")


Can we add a unit test for this use case?

…fier

mcook42 · 2024-11-05T18:42:38Z

jpo-ode-plugins/src/main/java/us/dot/its/jpo/ode/plugin/j2735/timstorage/Circle.java


-   @JsonProperty("position")


Is position an invalid property on a Circle Asn1Object in all cases, or is there any scenario where we want to have position present? I'm gathering that, at minimum, we don't want both position and center present, but is there a scenario where we could want position present when center is not (and vice versa)?

mcook42 · 2024-11-05T18:52:47Z

jpo-ode-svcs/src/test/java/us/dot/its/jpo/ode/traveler/TimTransmogrifierTest.java

+        ServiceRequest request = odeTID.getRequest();
+        request.setOde(new ServiceRequest.OdeInternal());
+        request.getOde().setVerb(ServiceRequest.OdeInternal.RequestVerb.PUT);
+        OdeTravelerInformationMessage tim = odeTID.getTim();
+        OdeMsgPayload timDataPayload = new OdeMsgPayload(tim);
+        OdeRequestMsgMetadata timMetadata = new OdeRequestMsgMetadata(timDataPayload, request);
+        timMetadata.setOdePacketID(tim.getPacketID());
+        SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'");
+        int maxDurationTime = 0;
+        Date latestStartDateTime = null;
+        for (OdeTravelerInformationMessage.DataFrame dataFrameItem : tim.getDataframes()) {
+            maxDurationTime = Math.max(maxDurationTime, dataFrameItem.getDurationTime());
+            latestStartDateTime = latestStartDateTime == null || latestStartDateTime.before(dateFormat.parse(dataFrameItem.getStartDateTime()))
+                    ? dateFormat.parse(dataFrameItem.getStartDateTime())
+                    : latestStartDateTime;
+        }
+        timMetadata.setMaxDurationTime(maxDurationTime);
+        timMetadata.setOdeTimStartDateTime(dateFormat.format(latestStartDateTime));
+        SerialId serialId = new SerialId();
+        serialId.setStreamId("testStreamId");
+        timMetadata.setSerialId(serialId);
+        timMetadata.setRecordGeneratedBy(OdeMsgMetadata.GeneratedBy.TMC);
+        timMetadata.setRecordGeneratedAt(DateTimeUtils.isoDateTime(DateTimeUtils.isoDateTime(tim.getTimeStamp())));
+        ObjectNode encodableTid = JsonUtils.toObjectNode(odeTID.toJson());
+        TravelerMessageFromHumanToAsnConverter.convertTravelerInputDataToEncodableTim(encodableTid);
+        timMetadata.setSchemaVersion(OdeProperties.OUTPUT_SCHEMA_VERSION);


suggestion: it looks like this code block is pulled from the depositTim method. I'm fine with pulling the logic out and duplicating it for this test, since the depositTim method is a Big Ball O' Mud ™️. It might be helpful for future maintainers if you added a comment explaining where this logic comes from and why it's necessary.

I've added a method-level comment explaining where the logic comes from and why it was pulled.

mcook42 · 2024-11-05T18:58:22Z

jpo-ode-svcs/src/test/java/us/dot/its/jpo/ode/traveler/TimTransmogrifierTest.java

+        String actualXML = TimTransmogrifier.convertToXml(null, encodableTid, timMetadata, serialId);
+
+        // verify
+        actualXML = actualXML.replaceFirst("<odeReceivedAt>.*</odeReceivedAt>", "<odeReceivedAt>2024-11-05T16:51:14.473Z</odeReceivedAt>"); // replace <odeReceivedAt> with a fixed value for comparison


suggestion(non-blocking): If we're editing the actual value, we're not truly testing that the code produces a specific output. It's not a huge deal to me since the rest of this code mocks the time value with an invalid timestamp🤢 instead. I like your solution here better than that, but I'd suggest looking at where the time value is provided and finding a way to use a static/fixed clock instead of rewriting the return value. I'm doing something like that over in my other PR, so feel free to copy that pattern, but it may be worth your time to consider a better approach.

drewjj

This makes sense to me

Michael7371

Looks good!

dmccoystephenson added 2 commits November 4, 2024 13:50

Fixed inconsistent json property annotating in Circle subclass of Asn…

9c9fe70

…1Object

Reverted changes to whitespace in header of Circle subclass of Asn1Ob…

27c2766

…ject

dmccoystephenson requested review from payneBrandon, mcook42 and Michael7371 November 4, 2024 21:27

dmccoystephenson commented Nov 4, 2024

View reviewed changes

Michael7371 requested changes Nov 4, 2024

View reviewed changes

dmccoystephenson marked this pull request as draft November 5, 2024 00:04

dmccoystephenson added 2 commits November 5, 2024 10:01

Added unit test to verify proper circle XML output from TimTransmogri…

a62ac30

…fier

Cleaned up new tim transmogrifier unit test

031d16c

dmccoystephenson requested a review from Michael7371 November 5, 2024 18:16

mcook42 approved these changes Nov 5, 2024

View reviewed changes

dmccoystephenson marked this pull request as ready for review November 5, 2024 19:12

Added comment for new test case in TimTransmogrifierTest

e57ccb3

drewjj approved these changes Nov 6, 2024

View reviewed changes

Michael7371 approved these changes Nov 14, 2024

View reviewed changes

dmccoystephenson added 2 commits November 19, 2024 17:08

Merge branch 'dev' into fix/update-circle-json-properties

29f4f38

Added missing imports in TimTransmogrifierTest

3cb982b

dmccoystephenson requested a review from mcook42 November 20, 2024 00:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed JSON annotations to enable ODE processing of circle geometries #114

Fixed JSON annotations to enable ODE processing of circle geometries #114

dmccoystephenson commented Nov 4, 2024 •

edited

Loading

dmccoystephenson Nov 4, 2024

Michael7371 Nov 4, 2024

dmccoystephenson Nov 5, 2024

mcook42 Nov 5, 2024

dmccoystephenson Nov 5, 2024

Michael7371 left a comment

Michael7371 Nov 4, 2024

mcook42 Nov 5, 2024

mcook42 Nov 5, 2024

dmccoystephenson Nov 5, 2024

mcook42 Nov 5, 2024

drewjj left a comment

Michael7371 left a comment

Fixed JSON annotations to enable ODE processing of circle geometries #114

Are you sure you want to change the base?

Fixed JSON annotations to enable ODE processing of circle geometries #114

Conversation

dmccoystephenson commented Nov 4, 2024 • edited Loading

PR Details

Description

Problem

Solution

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael7371 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewjj left a comment

Choose a reason for hiding this comment

Michael7371 left a comment

Choose a reason for hiding this comment

dmccoystephenson commented Nov 4, 2024 •

edited

Loading