Fix map #609

jwillmartin · 2024-04-18T21:27:30Z

PR Details

Description

Changes were made to handle files that could be expected. This includes:
Files with newline characters
Files with only MapData
Files containing a MessageFrame along with MapData

Plugin was also cleaned up with a header file.

Related Issue

#608

Motivation and Context

These changes make the MAP plugin more robust because it's able to handle MAP files that could be expected to be generated by a user.

How Has This Been Tested?

Tested using files with and without newline characters, files with and without MessageFrames, and a combination of the two.

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.
V2XHUB Contributing Guide
I have added tests to cover my changes.
All new and existing tests passed.

modified: src/v2i-hub/MapPlugin/src/MapPlugin.cpp new file: src/v2i-hub/MapPlugin/src/MapPlugin.h

dan-du-car · 2024-04-22T15:51:07Z

src/v2i-hub/MapPlugin/src/MapPlugin.cpp

+								fileContent.erase(remove(fileContent.begin(), fileContent.end(), '\n'), fileContent.end());
+
+								// Check for and remove MessageFrame
+								if (fileContent.size() >= 4 && fileContent.substr(0, 4) == "0012") {


What happens if there are multiple "38" in the binary payload? The same for 0012?

Great findings on these bugs!

@dan-du-car The message I used had 6 "38" in it. It only looked for the first instance, so this worked. But I can change it to make sure it only looks for it in the first couple of bytes depending on payload length.

I'll leave it as is. Find only looks for the first instance of the string.

dan-du-car · 2024-04-22T15:57:21Z

src/v2i-hub/MapPlugin/src/MapPlugin.cpp

+								if (fileContent.size() >= 4 && fileContent.substr(0, 4) == "0012") {
+									size_t pos = fileContent.find("38");
+									PLOG(logDEBUG) << "Beginning of MapData found at: " << pos;
+									fileContent.erase(0, pos);


Is this erase() starting from position 0 to one character before position "pos"? Or is this "pos" of a number of characters to be removed?

I would recommend to move this file reading and sanitizing steps into a separate function.

Good idea, I'll move it

It removes the number of pos bytes, starting from 0. So in this case removes bytes 0:[pos-1]

dan-du-car · 2024-04-22T16:01:19Z

src/v2i-hub/MapPlugin/src/MapPlugin.h

+		return tmx::message::to_tree(static_cast<tmx::message>(m));
+	}
+
+	static MapFile from_tree(tmx::message_tree_type tree) {


Are these from_tree and to_tree functions used?

Doesn't look like it. They were also included in another the inputs/ISDDataAdaptor.hpp and also not used. I'll remove it and retest

It was needed by message.hpp in TmxApi. Left it in

dan-du-car · 2024-04-22T16:01:54Z

src/v2i-hub/MapPlugin/src/MapPlugin.h

+
+	std_attribute(this->msg, int, Action, -1, );
+	std_attribute(this->msg, std::string, FilePath, "", );
+	std_attribute(this->msg, std::string, InputType, "", );


Is this "inputType" used? What does this represent?

Looks like it was declared but is expected to be empty? I'll find out if we need it or not

It's needed to find the file extension type for ISD, TXT, XML, JSON, UPER. Kept it in

Cool. What is the difference between UPER and TXT upload? It seems they both upload bytes but TXT has message frame in it? Why do we need these two different uploads that has similar content? I am not familiar with the usage of this plugin, so I am a bit curious about these different extension types?

UPER is used to accept the message frame, but it assumes the message is correct and broadcasts it to TMX core.
I made changes to the TXT to accept the message frame, but it strips it and decodes the remaining content to verify the message

I think any format we accept should very message content before broadcast. When it is not verified we get exceptions on down stream plugins that attempt to ingest this message. I created the TXT format a while back to avoid dealing with issues with the UPER format logic. These two formats can likely be consolidated since they are both uper hex

dan-du-car · 2024-04-22T16:03:10Z

src/v2i-hub/MapPlugin/src/MapPlugin.h

+	MapFile(): tmx::message() {}
+	virtual ~MapFile() {}
+
+	std_attribute(this->msg, int, Action, -1, );


What is the rest of actions this MAP support? Which default action is this initialized to?

Default is no action existing. It looks like only action is for finding the file path.

modified: src/v2i-hub/MapPlugin/src/MapPlugin.h

paulbourelly999 · 2024-05-01T13:56:13Z

src/v2i-hub/MapPlugin/src/MapPlugin.cpp

@@ -304,6 +207,17 @@ int MapPlugin::Main() {
 	return (EXIT_SUCCESS);
 }

+string MapPlugin::removeMessageFrame(string &fileContent) {
+    // Check for and remove MessageFrame
+	if (fileContent.size() >= 4 && fileContent.substr(0, 4) == "0012") {


Pull from MAP PSID constant

Resolved in latest commit

modified: src/v2i-hub/MapPlugin/src/MapPlugin.h

paulbourelly999 · 2024-05-28T14:48:45Z

src/v2i-hub/MapPlugin/src/MapPlugin.cpp

+    string map_messageID = enum_to_hex_string();
+
+    // Check for and remove MessageFrame
+    if (fileContent.size() >= 4 && fileContent.substr(0, 4) == map_messageID)


I don't think this is a very robust way to determine whether the message contains the message frame or not. I think it would be more robust if we initially assumed the header was there and tried to decode it, if we run into an exception we log the exception as a warning and log that we are retrying without the header. Having code in our plugin that is couple to the exact format of the MAP message means that we have to rewrite it every-time the exact format changes. Having code that is loosely couple to just the C Struct and data means that the format can change without require logic changes here.

We could do that, but we'd still have to have logic to figure out whether the header has an additional 2 bytes or not. The message frame will always contain the message ID, at the beginning so this is a good way to quickly check for it instead of trying and failing to encode the message. I think both ways would work, but I'd rather run this check instead of attempting a blind encoding.

paulbourelly999 · 2024-05-28T14:50:59Z

src/v2i-hub/MapPlugin/src/MapPlugin.h

+
+	bool LoadMapFiles();
+	void DebugPrintMapFiles();
+	string enum_to_hex_string();


What is the purpose of this method?

It's to convert the messageID from a decimal enum to a hex string. The only place the message ID is declared in TmxApiMessages

paulbourelly999 · 2024-05-28T14:52:26Z

src/v2i-hub/MapPlugin/src/MapPlugin.h

+	bool LoadMapFiles();
+	void DebugPrintMapFiles();
+	string enum_to_hex_string();
+	string removeMessageFrame(string &fileContent);


Left an earlier comment about how I think this would be better implemented.

Responded in other comment

paulbourelly999 · 2024-05-28T14:53:26Z

src/v2i-hub/MapPlugin/src/MapPlugin.cpp

@@ -304,6 +207,39 @@ int MapPlugin::Main() {
 	return (EXIT_SUCCESS);
 }

+string MapPlugin::enum_to_hex_string()
+{
+	sprintf(mapID_buffer, "%04X", tmx::messages::api::mapData);


What happens if this method is called and mapID_buffer is not initialized or garbage initialized?

Initialized with zeros in header file. Missed the {0}, but just added it.

paulbourelly999 · 2024-05-28T14:54:43Z

src/v2i-hub/MapPlugin/src/MapPlugin.cpp

+	return map_messageID;
+}
+
+string MapPlugin::removeMessageFrame(string &fileContent)


What happens if this method is called and fileContent is not initialized or null initialized.

fileContent is passed by reference and the method is only called after fileContent has already been initialized. Let me know if you want to add anything else to it to make sure this method doesn't crash the plugin

paulbourelly999

I requested some changes here. Please let me know if this story/PR is growing more in scope and needs some kind of story associated with it. I can help implementing the changes as well if needed.

paulbourelly999 · 2024-05-28T14:56:53Z

src/v2i-hub/MapPlugin/src/MapPlugin.cpp

-using namespace tmx::messages;
-using namespace tmx::utils;
+#include "MapPlugin.h"
+#include <chrono>


Why are these includes not moved to the header.

Good catch, moved over to header.

paulbourelly999 · 2024-05-28T15:37:42Z

src/v2i-hub/MapPlugin/src/MapPlugin.h

@@ -0,0 +1,111 @@
+#pragma once


#pragma once

is a way to guard against against headers being included multiple times. This inclusion guard below does the same thing

#ifndef MAPPLUGIN_H_ #define MAPPLUGIN_H_

There is no reason to do both. Just pick one (https://learn.microsoft.com/en-us/cpp/preprocessor/once?view=msvc-170)

Kept is as #pragma once, thanks!

modified: src/v2i-hub/MapPlugin/src/MapPlugin.h

paulbourelly999 · 2024-05-30T12:55:43Z

src/v2i-hub/MapPlugin/src/MapPlugin.cpp

+	return map_messageID;
+}
+
+string MapPlugin::removeMessageFrame(string &fileContent)


This should be const string &filcontent

paulbourelly999

There are a couple more things I would like fixed inside the MAP plugin under this issue as long as this issue is not blocking some other work or stakeholder. I think we should consolidate TXT and UPER formats and avoid any direct byte reading or changing. We should verify any message and possibly log (tmx-event) if we are unable to verify a given message.
If you believe we can do some of this later, if you could create a follow on story for the MAP plugin to consolidate these two formats that would be create. Lastly I just want to confirm that both TXT and UPER have been tested for functionality before merging this.

modified: src/v2i-hub/MapPlugin/src/MapPlugin.h

sonarcloud · 2024-06-27T19:03:14Z

Quality Gate failed

Failed conditions
0.0% Line Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

jwillmartin added 6 commits April 17, 2024 18:35

modified: src/v2i-hub/MapPlugin/manifest.json

0ff8c7a

modified: src/v2i-hub/MapPlugin/src/MapPlugin.cpp new file: src/v2i-hub/MapPlugin/src/MapPlugin.h

modified: src/v2i-hub/MapPlugin/src/MapPlugin.cpp

f2f5d45

modified: src/v2i-hub/MapPlugin/src/MapPlugin.cpp

2a9ff27

modified: src/v2i-hub/MapPlugin/src/MapPlugin.cpp

64a451c

Merge remote-tracking branch 'origin' into fix-MAP

1d2e0cc

modified: src/v2i-hub/MapPlugin/src/MapPlugin.cpp

d298723

jwillmartin requested review from paulbourelly999 and dan-du-car April 18, 2024 21:27

dan-du-car reviewed Apr 22, 2024

View reviewed changes

modified: src/v2i-hub/MapPlugin/src/MapPlugin.cpp

cfc8b43

modified: src/v2i-hub/MapPlugin/src/MapPlugin.h

jwillmartin requested a review from dan-du-car April 29, 2024 20:59

paulbourelly999 reviewed May 1, 2024

View reviewed changes

dan-du-car approved these changes May 24, 2024

View reviewed changes

modified: src/v2i-hub/MapPlugin/src/MapPlugin.cpp

65a9e42

modified: src/v2i-hub/MapPlugin/src/MapPlugin.h

jwillmartin requested review from paulbourelly999 and dan-du-car May 24, 2024 16:51

paulbourelly999 reviewed May 28, 2024

View reviewed changes

paulbourelly999 requested changes May 28, 2024

View reviewed changes

paulbourelly999 reviewed May 28, 2024

View reviewed changes

modified: src/v2i-hub/MapPlugin/src/MapPlugin.cpp

32dc37b

modified: src/v2i-hub/MapPlugin/src/MapPlugin.h

jwillmartin requested a review from paulbourelly999 May 29, 2024 22:33

paulbourelly999 reviewed May 30, 2024

View reviewed changes

paulbourelly999 requested changes May 30, 2024

View reviewed changes

jwillmartin added 6 commits June 4, 2024 14:27

Merge remote-tracking branch 'origin' into fix-MAP

fce0eda

modified: src/v2i-hub/MapPlugin/src/MapPlugin.cpp

fa843d0

modified: src/v2i-hub/MapPlugin/src/MapPlugin.h

modified: src/v2i-hub/MapPlugin/src/MapPlugin.cpp

aef5746

modified: src/v2i-hub/MapPlugin/src/MapPlugin.h

modified: src/v2i-hub/MapPlugin/src/MapPlugin.h

7bbf823

Merge branch 'develop' into fix-MAP

bc83c6c

modified: src/v2i-hub/MapPlugin/src/MapPlugin.cpp

7de6bbd

modified: src/v2i-hub/MapPlugin/src/MapPlugin.h

jwillmartin requested a review from paulbourelly999 June 27, 2024 19:04

paulbourelly999 approved these changes Jun 27, 2024

View reviewed changes

paulbourelly999 merged commit f933ff7 into develop Jun 27, 2024
2 of 3 checks passed

paulbourelly999 deleted the fix-MAP branch June 27, 2024 19:59

Fix map #609

Fix map #609

Conversation

jwillmartin commented Apr 18, 2024

PR Details

Description

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-du-car Apr 30, 2024 • edited Loading

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

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

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

paulbourelly999 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

paulbourelly999 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 27, 2024

Quality Gate failed

dan-du-car Apr 30, 2024 •

edited

Loading