OdeTimJson POJO Processing Rework #119

drewjj · 2024-11-25T07:14:19Z

PR Details

Description

Addresses a number of inconsistencies between the ODE created and decoded TIM messages by creating a new standard OdeTimJson POJO (OdeTimData). This new POJO has a set of builder classes to ensure that all TIM XML is processed into the POJO correctly and consistently. These builder classes were created in the same fashion as the older ODE builders but improvements have been made to reduce the number of redundant wrapper objects around arrays. This has caused some small changes around other message types that utilize NodeXY, specifically Map.

Summary of changes:

New TIM POJO OdeTimData
New OdeTimData builder class and helper
Modifications to the Asn1DecodedDataRouter and TimDepositController to utilize the new POJO when writing to the topic.OdeTimJson topic
OdeTimJson output no longer includes redundant array wrapper classes for TravelerDataFrame, GeographicalPath, NodeXY, and NodeLL
Map is affected by the removal of the redundant wrapper class NodeXY and no longer includes it
TIM and Map schemas have been updated accordingly
Unit tests have been added and updated

Related Issue

No related jpo-ode issue.

Motivation and Context

The purpose of this change is to resolve an inconsistency between the output of the decoded TIM messages and the jpo-ode TIM endpoint created TIM messages. There is no prior TIM POJO to guarantee a consistent, standard output for the topic.OdeTimJson data. This also allows for other projects to easily deserialize the TIM output of the jpo-ode.

The changes to the redundant object wrappers is to make the TIM output more closely resemble the J2735 standard.

How Has This Been Tested?

Unit tests, approval tests, and local testing with real TIM and Map data.

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.

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

jpo-ode-plugins/src/main/java/us/dot/its/jpo/ode/plugin/j2735/builders/NodeListXYBuilder.java

mcook42

I like the work here! The changes here are a big improvement and much appreciated. A special thanks to you for adding more tests. There are some requested changes and some general suggestions/chores as you'll see below.

But first, overall, I'm unsure why we aren't leveraging Jackson and the annotations to enable de/serialization. It would make classes/code like we have in ComputedLaneBuilder unnecessary by allowing Jackson to de/serialize the objects correctly. I understand the work to make that change is outside of the scope of this PR, but I don't want it to go unmentioned. If we migrate to the standard practice of using Jackson for POJO de/serialization, we could avoid the needing to perform these bug fixes and avoid hidden issues we haven't encountered yet. Do you know if there's there a reason why we don't use Jackson annotations?

If we don't want to annotate the POJOs themselves (not sure why we'd avoid that since we own and maintain the POJOs) we could use MixIns.

General Suggestions:

wherever we have something like private static final long serialVersionUID = 1L; we should consider adding the @Serial annotation to allow the java compiler to check for correctness (see java doc)
Whenever we are creating or updating POJOs, we should consider using the Lombok annotations @Getter and @Setter or the more comprehensive @Data. This will reduce boilerplate code and the risk of typo-related bugs (and it saves the writer and reviewer time because we don't need to review the getters and setters)
Run a quick reformat on all changed files so that the formatting is consistent within and across the files
We don't need to explicitly catch and fail in tests. We can just add a throws to the test method signature and JUnit will handle that for us
Unit test methods and classes don't need access modifiers unless we're doing something special, so you can (and probably should) leave out the public modifiers when writing the tests It is generally recommended to omit the public modifier for test classes, test methods, and lifecycle methods unless there is a technical reason for doing so (ref)

General chores:

Whenever we are modifying/creating an enum class, we should be following the standard naming convention of UPPER_SNAKE_CASE: all uppercase letters, with each word separated from the next by a single underscore. If we need to serialize (stringify) the values to a lowercase non-snack-case string, we can define an overridden toString method to do this for us or use an internal string property like String value that lets us define the stringified value on enum initialization. I've provided some examples of both throughout the PR (ref

jpo-ode-core/src/test/java/us/dot/its/jpo/ode/model/OdeTimDataTest.java

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

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

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

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

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

jpo-ode-plugins/src/main/java/us/dot/its/jpo/ode/plugin/j2735/builders/ComputedLaneBuilder.java

jpo-ode-plugins/src/test/java/us/dot/its/jpo/ode/plugin/j2735/builders/TimBuilderTest.java

jpo-ode-svcs/src/main/java/us/dot/its/jpo/ode/services/asn1/Asn1EncodedDataRouter.java

iyourshaw · 2024-11-30T22:04:22Z

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

+public class J2735GeometricProjection extends Asn1Object {
+    private static final long serialVersionUID = 1L;
+
+    private J2735HeadingSlice direction;


Should be a J2735BitString here, because HeadingSlice is a BitString. Need to be able to represent more than one value being set at the same time.

…namically created POJOs

…quences

…est case for testing deserialization of XER and serialization into JSON

… and dWidth properties. Fix OdeTimDataCreatorHelperTest data

github-actions

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

checkstyle

jpo-ode-core/src/test/java/us/dot/its/jpo/ode/model/OdeTimDataTest.java

jpo-ode-core/src/main/java/us/dot/its/jpo/ode/model/OdeTimPayload.java

jpo-ode-svcs/src/test/java/us/dot/its/jpo/ode/coder/OdeTimDataCreatorHelperTest.java

jpo-ode-svcs/src/main/java/us/dot/its/jpo/ode/services/asn1/Asn1EncodedDataRouter.java

jpo-ode-plugins/src/test/java/us/dot/its/jpo/ode/plugin/j2735/builders/MAPBuilderTest.java

drewjj · 2024-12-06T08:16:11Z

I addressed all of the checkstyle issues. I did tell it to ignore the generated J2735 files, though. This is a pretty normal thing to do with generated code since it will just get overwritten if we ever need to generate a new standard.

Just need to update the schema, revert the changes to Map, and then those unit tests should be good.

drewjj · 2024-12-06T11:19:19Z

It is complaining about the diff being too large and the checkstyle can't run due to it. The checkstyle worked in the previous commit so I would like to assume everything is fine there so this shouldn't be a blocker.

…JSON, change license boilerplate to a non-javadoc comment at top of files.

mcook42

Question(non-blocking): Is there a reason we put the new models under the jpo-ode-plugins package instead of jpo-ode-common or jpo-ode-core?

jpo-ode-svcs/src/main/java/us/dot/its/jpo/ode/coder/OdeTimDataCreatorHelper.java

jpo-ode-svcs/src/test/java/us/dot/its/jpo/ode/coder/OdeTimDataCreatorHelperTest.java

drewjj · 2024-12-09T21:11:02Z

Question(non-blocking): Is there a reason we put the new models under the jpo-ode-plugins package instead of jpo-ode-common or jpo-ode-core?

The idea was to put it in the same place as the other non-generated J2735 POJOs. We could move this somewhere else down the road but I think it makes sense for the present moment.

…k properly. Updates to affected unit tests

Michael7371

I really like the output schema on this TIM refactor! Nice work.

Michael7371 · 2024-12-10T19:12:34Z

jpo-ode-core/src/main/java/us/dot/its/jpo/ode/model/OdeTimData.java

+   public void setPayload(OdeMsgPayload payload) {
+      super.setPayload(payload);
+   }
+


I added these overrides to the OdeTimData.java class to allow for serialization using this class as the parent.

Sounds great, thanks!

mwodahl

This looks good to me! Unit tests all look good and everything worked correctly when running locally.

drewjj added 6 commits November 21, 2024 21:34

Add TIM POJO and builder

2e1c179

Modify TimCreator to use the new creator. Add and update unit tests

5effd8a

Merge remote-tracking branch 'origin/dev' into tim-pojo-rework

69f5137

Update schemas

d93e6ec

Add support for unsigned TIMs to go to the TMC filtered topic

6848ede

Update unit tests to address recent changes

8b8de0c

iyourshaw reviewed Nov 27, 2024

View reviewed changes

jpo-ode-plugins/src/main/java/us/dot/its/jpo/ode/plugin/j2735/J2735NodeListXY.java Outdated Show resolved Hide resolved

iyourshaw reviewed Nov 27, 2024

View reviewed changes

jpo-ode-plugins/src/main/java/us/dot/its/jpo/ode/plugin/j2735/builders/NodeListXYBuilder.java Show resolved Hide resolved

mcook42 requested changes Nov 27, 2024

View reviewed changes

iyourshaw reviewed Nov 30, 2024

View reviewed changes

drewjj and others added 20 commits December 2, 2024 16:54

Remove the array from the computed lane

595f47c

Add dynamic serializing and deserializing of TIM messages with new dy…

f3d5cf6

…namically created POJOs

Add @JsonIgnore to fields on asn1 type base classes

5c1892e

Serialize Bitstrings as verbose maps for ODE JSON

f4a85e6

Set extension presence booleans to jsonignore annotations

3603e0f

BitString serialization rules to build them out as verbose boolean se…

7c991f5

…quences

Bitstring deserializer for verbose ODE json

c5e7bda

Remove diagnostic logging statements

578e2cc

Remove printf statements

651941e

Set tim udp buffer size to 2048. Add complex tim example to script

afee483

Complex example tims

235f51a

Comment manual edits

ef385c6

Java style: use Lobok for getters/setters, capitalize enum items

2568fab

Use 2020/2024 modules as package names. Package names lower case.

f223988

Fix test namespace

ff5a742

Merge remote-tracking branch 'iy/tim-pojo-rework' into tim-pojo-rework

29d7bf6

Remove unused older POJOs and update the TIM test with a better XML t…

c726baa

…est case for testing deserialization of XER and serialization into JSON

Merge remote-tracking branch 'origin/dev' into tim-pojo-rework

2b8ebcb

Add test for NodeAttributeSetLL. Fix JSON serialization of dElevation…

37e5399

… and dWidth properties. Fix OdeTimDataCreatorHelperTest data

Merge remote-tracking branch 'origin/dev' into tim-pojo-rework

921b05e

drewjj added 2 commits December 5, 2024 23:35

Code styling changes to appease the code styling lords

f676a6b

Update checkstyle xml

2fc4615

github-actions bot reviewed Dec 6, 2024

View reviewed changes

Even more code styling changes

ce5538a

github-actions bot reviewed Dec 6, 2024

View reviewed changes

Last bit of code styling. Please.

fb4f577

Update the TIM schema and resolve some test issues

abce4e5

iyourshaw and others added 3 commits December 8, 2024 23:47

Regenerated TIM classes: Fix for sequence-of choice serialization to …

48fea3a

…JSON, change license boilerplate to a non-javadoc comment at top of files.

itis, region, fix tests

a397df0

Update attribution documentation to note where the TIM POJOs came from

92c4504

mcook42 approved these changes Dec 9, 2024

View reviewed changes

drewjj added 2 commits December 9, 2024 14:07

Address pull request comments

8c35ad2

Update the imported assertEquals

7dccf75

drewjj requested review from mwodahl and Michael7371 December 9, 2024 21:17

mcook42 approved these changes Dec 9, 2024

View reviewed changes

update to TimData class to allow for Tim payload serialization to wor…

9e48c45

…k properly. Updates to affected unit tests

Michael7371 approved these changes Dec 10, 2024

View reviewed changes

mcook42 approved these changes Dec 11, 2024

View reviewed changes

dmccoystephenson self-requested a review December 11, 2024 15:34

mwodahl approved these changes Dec 11, 2024

View reviewed changes

drewjj merged commit cd232b6 into dev Dec 11, 2024
4 of 5 checks passed

drewjj deleted the tim-pojo-rework branch December 11, 2024 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OdeTimJson POJO Processing Rework #119

OdeTimJson POJO Processing Rework #119

drewjj commented Nov 25, 2024

mcook42 left a comment

iyourshaw Nov 30, 2024 •

edited

Loading

github-actions bot left a comment

drewjj commented Dec 6, 2024

drewjj commented Dec 6, 2024 •

edited

Loading

mcook42 left a comment

drewjj commented Dec 9, 2024 •

edited

Loading

Michael7371 left a comment

Michael7371 Dec 10, 2024

drewjj Dec 11, 2024

mwodahl left a comment

OdeTimJson POJO Processing Rework #119

OdeTimJson POJO Processing Rework #119

Conversation

drewjj commented Nov 25, 2024

PR Details

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mcook42 left a comment

Choose a reason for hiding this comment

iyourshaw Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

drewjj commented Dec 6, 2024

drewjj commented Dec 6, 2024 • edited Loading

mcook42 left a comment

Choose a reason for hiding this comment

drewjj commented Dec 9, 2024 • edited Loading

Michael7371 left a comment

Choose a reason for hiding this comment

Michael7371 Dec 10, 2024

Choose a reason for hiding this comment

drewjj Dec 11, 2024

Choose a reason for hiding this comment

mwodahl left a comment

Choose a reason for hiding this comment

iyourshaw Nov 30, 2024 •

edited

Loading

drewjj commented Dec 6, 2024 •

edited

Loading

drewjj commented Dec 9, 2024 •

edited

Loading