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

Feature/refactor set #113

Merged
merged 30 commits into from
Mar 9, 2021

Conversation

SebastianSchildt
Copy link
Contributor

This is work in progress.

Replace old set code with streamlined code supporting "/" paths.

Advanced features such as multiset in a later step, when migrating msg semantic to Gen2

Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Copy link
Contributor

@wenwenchenbosch wenwenchenbosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like, that many code is removed. I will test the code now

test/unit-test/VssDatabaseTests.cpp Show resolved Hide resolved
src/VssDatabase.cpp Show resolved Hide resolved
src/VssDatabase.cpp Outdated Show resolved Hide resolved
src/VssCommandProcessor.cpp Show resolved Hide resolved
@wenwenchenbosch
Copy link
Contributor

Issue 1:

VSS Client> setValue Vehicle.Speed 200
{
    "action": "set", 
    "requestId": "0bc7f8af-7e9d-46c9-8de3-21d778b1d168", 
    "timestamp": 1613404411366
}

VSS Client> setValue Vehicle.Speed2 200
{
    "action": "set", 
    "requestId": "02eeebf2-f6fe-44f2-aefb-4dbd8ba30493", 
    "timestamp": 1613404413198
}

VSS Client> setValue bla 200
{
    "action": "set", 
    "requestId": "0a8883b9-546b-4e59-8ed6-4871ca7fb4e4", 
    "timestamp": 1613404429551
}

@wenwenchenbosch
Copy link
Contributor

expected behavior:

VSS Client> setValue Vehicle.Speed2 200
{
    "action": "set", 
    "error": {
        "message": "I can not find Vehicle.Speed2 in my db", 
        "number": 404, 
        "reason": "Path not found"
    }, 
    "requestId": "63ca27e8-1afe-457c-a9cf-56c2d246c38c", 
    "timestamp": 1613404486744
}

@wenwenchenbosch
Copy link
Contributor

issue 2: mqtt publisher does not work any more :(

SebastianSchildt and others added 11 commits February 17, 2021 12:13
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Chen Wenwen (CR/AME5) <wenwen.chen@de.bosch.com>
Signed-off-by: Chen Wenwen (CR/AME5) <wenwen.chen@de.bosch.com>
Signed-off-by: Chen Wenwen (CR/AME5) <wenwen.chen@de.bosch.com>
Signed-off-by: Chen Wenwen (CR/AME5) <wenwen.chen@de.bosch.com>
Signed-off-by: Chen Wenwen (CR/AME5) <wenwen.chen@de.bosch.com>
Signed-off-by: Chen Wenwen (CR/AME5) <wenwen.chen@de.bosch.com>
Signed-off-by: Chen Wenwen (CR/AME5) <wenwen.chen@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
@SebastianSchildt
Copy link
Contributor Author

Now non-existing paths can not be set anymore. Fresh unittestests still missing....

Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
@SebastianSchildt SebastianSchildt marked this pull request as ready for review March 8, 2021 16:39
try {
database->setSignal(channel, path, request["value"], gen1_compat_mode);
for ( std::tuple<VSSPath,jsoncons::json> setTuple : setPairs) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SebastianSchildt You looped setPairs three times!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was (that is what I told myself) going for readability instead of performance and deeper nesting. I hope the datastructure with the vector of tuples thing access compiles down to just moving a pointer, but I do not know 😕
Maybe we add an enhancement issue to look into that later?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right with readability. But in this case, I do not think, that putting everything inside one loop is less readable than using 3 loops. If you like to improve the readability, better refact the method with multiple functions. It is always best practices to have smaller function/method, better than a function with more than 100 lines of code. But If you like, you can create an issue for this, instead of fixing it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

)"};
jsoncons::json expectedJson = jsoncons::json::parse(expectedJsonString);

// run UUT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is UUT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the style form the existing tests, i assume "Unit under test"

"kuksa-vss": {
"Vehicle.OBD.*": "rw"
}
}
*/
string AUTH_TOKEN = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJFeGFtcGxlIEpXVCIsImlzcyI6IkVjbGlwc2Uga3Vrc2EiLCJhZG1pbiI6dHJ1ZSwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjE2MDkzNzI4MDAsInczYy12c3MiOnsiVmVoaWNsZS5PQkQuKiI6InJ3In19.LLdJFIuoViNF4uVv1IL30fXPySoM1oCHxLrWm6xM_4eynvXqPvrCI9fW__GMI0d8PxBLpM8FhgG3ynVVlOuLV_Sl6lDImZlkNQiR02lJwFqf67RWVdI4f4uhdHdKjEpe0a0F-6e7McS__3qQYyjNuQAZIIXkZUIDyXye9upwNARj1wGPtZyNzSY1uyxmuc7MMPaILAIzL8ZnY_D9qgbpbiInGavZtDE_X1iy9GhxbUguP8oiVYn14-H6RBDIF0s5dXwXnJ0cm9Q2DTFpb0YRq4sMgTC4PT1Smdda_6Pj2ELmBjGbH7PYlqfVk1jVdSPGcUpU48e__qVitSRkEK_unM5CihrDVIy7nw3_KclIZJa8_af3kQ4x-leg-ErRCt78j5l0DDxIo5EtCxAeLbO7baZD6D1tPrb3vYkI_9zl1vzydp_nmNMS9QzCRq5yEJfP07gpvG0Z1O0tSLedSCG9cZJ-lJ3Oj3bqxNxk3ih6vKfjnvjAgli7wEP_etHofZmqs3NI-qtP6ldz93fBfAK_iApsSnG4PV7oS_-3UQIow6fUHAA8szn4Ad1ZYiaDsXRcHbdIoLEemGDllkRTYNBe_5vFDT3s1gY82L3fvgwAzTGZ2k46eh66Zx3SmuPgHlCQK6gR-6eAVn0jh_Tjk5vubtin6UdRjHF0Y4BvCwvE0bk";
string AUTH_TOKEN = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJFeGFtcGxlIEpXVCIsImlzcyI6IkVjbGlwc2Uga3Vrc2EiLCJhZG1pbiI6dHJ1ZSwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjE3MDkzNzI4MDAsImt1a3NhLXZzcyI6eyJWZWhpY2xlLk9CRC4qIjoicncifX0.U4zpy38DBTx2thPWfOMKALfZovJdkr5xQWDQaGSQo6sMhG-MgDz2UZDvVyRllHiv76f2VmZ2X0TnUZ_Z6yFUmjVYq83LGzUjSDY-xFLLRqOPvFKsQbTmOLbT629uU5Pz2m6x1HcxHBVCxNvI3pZw1D_rHODkVVlgSjycDK5NaHi9EdbSDmccmwk3jI6tPag0eKC7adzBlqkt-h6vqlx_FRz7kjyhiLXmR95QIV3_0MIyq484SsU4ztMPgmHBn1VUWz0x5EW065J2BIavXpaF20oNek0ZzsGcDq-UEQOLPDFoAnYykFrcYmIlXc51wRU2Mw-IIIIs4CV9-Vp1GkQfhQCicmR3OprRepzi_C0Fdh6kwSetAYJWqlVOeo1oUIxFIYbxGFjtdG38dOaacDddBnJ93K8JlnbFdp79Ins39HCmRPit198vdf7gLRcM_GLtvGF6SbT0V2tn17CNt2UxLEniHp7hGybxBVKqgTws2H73ZLe4HEq3UU8vhbeg2V2KXeSXvDJeE3LZvDWARafyHVXrkOx8-O9HqIClfAr-LAl7nnBqq2f9DneW1hjFZOFV5RUCrJ_UpZR_hyEU_aroXO6MNw_MC6KvZeOC6ti1NO6uAB3BYHzpNP1mlyUHlfRl-oLuKZFFqFVBrxY3tBrQwgzfhPZwi427ZIwrNlfv-aU";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was an old token (see https://jwt.io/), the old code just never checked it (there are ealier and additional access checks now, in a place that is not mocked in this test unit. as the test already provided a token (albeit it was irrelevant) I replaced it with a validone, so the test goes through)

Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
@SebastianSchildt SebastianSchildt merged commit fcca80f into eclipse:master Mar 9, 2021
@wenwenchenbosch wenwenchenbosch deleted the feature/refactor_set branch March 17, 2021 11:25
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.

2 participants