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

Handle omitted parameter permissions properly in OnVehicleData notification #3590

Merged
merged 2 commits into from
Dec 10, 2020

Conversation

jacobkeeler
Copy link
Contributor

Fixes #3577

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Run ATF tests listed in #3577 (comment)

Summary

Allow all parameters in OnVehicleData if parameters field is omitted

Changelog

Bug Fixes
  • Allow all parameters in OnVehicleData if parameters field is omitted

CLA

Copy link
Collaborator

@iCollin iCollin left a comment

Choose a reason for hiding this comment

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

please run style script also

120,121c120
<     }
<     else {
---
>     } else {
in  src/components/application_manager/rpc_plugins/vehicle_info_plugin/src/commands/mobile/on_vehicle_data_notification.cc

Comment on lines +116 to +117
parameters_permissions_.disallowed_params.empty() &&
parameters_permissions_.undefined_params.empty()) {
Copy link
Collaborator

@iCollin iCollin Dec 4, 2020

Choose a reason for hiding this comment

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

if i set these all to empty arrays, all params should be allowed, correct?

is there any way to differentiate between the case of all being defined as [] and the whole object missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the way that it is checked in CommandImpl::CheckAllowedParameters, I believe that all of the provided parameters would be in the disallowed_params list in the case that it is defined as [], so these lists would not be empty.

@dboltovskyi
Copy link
Contributor

dboltovskyi commented Dec 14, 2020

@jacobkeeler Please notice the following scripts start failing within this fix:

./test_scripts/API/VehicleData/OnVehicleData/002_OnVD_disallowed.lua
./test_scripts/API/VehicleData/OnVehicleData/003_OnVD_disallowed_after_PTU.lua

SDL Core: https://github.com/smartdevicelink/sdl_core/tree/develop (2e5c3ab)
ATF Report and Logs:

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.

3 participants