Sdl_evolution: [Accepted] SDL 0336 - Strict Versioning for Outgoing Core Messages

[theresalech]

Hello SDL community,

The review of the revised proposal "SDL 0336 - Strict Versioning for Outgoing Core Messages" begins now and runs through June 29, 2021. Previous reviews of this proposal took place May 19 - June 1, 2021 and June 9 - June 15, 2021. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0336-strict-versioning-for-outgoing-core-messages.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

https://github.com/smartdevicelink/sdl_evolution/issues/1144

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

• Is the problem being addressed significant enough to warrant a change to SDL?
• Does this proposal fit well with the feel and direction of SDL?
• If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
• How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
  Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you,
Theresa Lech

Program Manager - Livio
[email protected]

[joeljfischer]

1. I think that when a parameter or structure is dropped on Core, the message should be change from SUCCESS to WARNINGS and an info string should be added mentioning what was dropped and why (like "gps parameter was dropped due to a RPC version mismatch, please update the app library to fix this issue").

[joeygrover]

1. Would this be breaking for older apps? Since they will expect the success flag to be set to true and for the response code to equal SUCCESS. Changing that to a WARNINGS result code implies something went wrong even though every piece of the message otherwise aligns with the RPC spec version that was negotiated. Adding that info or similar to the info string makes sense, but I disagree with changing the result code as I believe it is a bit misleading and a potential breaking change for an app developer.

[joeljfischer]

1. In the app libraries (AFAIK, at least in the iOS lib), WARNINGS is considered a successful RPC response for all intents and purposes. The success parameter is still true (which is what devs should be using to determine RPC success). The only effective difference is to tell the app developer that something didn't go perfectly and to tell the app library to print a log. To me, both of those things should be happening in this case.

The bigger question, I think, is whether this proposal itself is "breaking" (scare quotes intentional, because it's not _actually_ breaking). If, before this proposal, a developer depended on the gps parameter coming back from a v7.0.0 system, and their code didn't break because they never accessed the mandatory (to them) satellites parameter. With this change, when connecting to a v7.X.0 system, the gps parameter will no longer be sent, potentially disrupting their use case until they make a major version update to the app library. Before, the developer could assume that gps could be there, now it's guaranteed that it won't on any system running with this code.

[joeygrover]

1. I disagree, because nothing went wrong. The RPC spec was negotiated to a specific version and that version is what is being used. Implying that Core removing a parameter that was added after the negotiated RPC spec version as something that has gone wrong is incorrect. The message to the developer should be that everything succeeded as expected based on the mutually agreed upon RPC spec version, but there is more available when using a newer spec. Changing the result code is misleading towards this message, but I'm ok with adding it into the info param. That would allow the developer to be informed without a potential breaking change.

We can't rely on only what the app libraries do or how they process the RPCs since app developers are more than able to send/receive them on their own and would likely include other RPCs that the library currently only passes through.

I do not believe the two hypothetical developer situations being discussed are equivalent. The usage of the result code is an accepted use of parameters as defined in the RPC spec, changing the definition of that code for that spec version is a breaking change. Using a parameter that was not included in the spec version that the app stated it supported and was negotiated does not follow the RPC spec and therefore is not a protected case.

[jacobkeeler]

1. Agreed that an info message would be good here, this is also similar to SDL-0298 handles this (although sadly this wouldn't be possible for notifications). As Joel noted, a WARNINGS result with success:true is generally standard practice when sending a warning message from Core to an app, though to Joey's point it does get a bit murky when it's intended behavior and the application is unable to do anything to fix it. I'm reasonably fine with sending SUCCESS with an info field in this scenario, though it is different from standard Core behavior.

As for Joel's second point, there are definitely some tradeoffs in this proposal (flexibility vs. correctness), and this solution could definitely restrict some pieces of functionality in older apps. Unfortunately I don't know if there really is a middle-ground solution to this which still guarantees that Core will alway obey the negotiated spec version.

[joeygrover]

It appears both of us were in a little off in our understanding of the proposal and we've come to an agreement that we need to work towards a more complicated solution. How Core works now is not good, but the proposal will also change items that would cause some headaches to developers. For example the gps parameter being omitted because of a missing sub param but the response/notification being successful is disingenuous to the developer.

I believe a going by a case by case basis is the only way to create a solution that properly aligns to the RPC spec as well as provide the best usability for developers. So if we look at the examples given in the proposal that gives us a few types of modifications that we can create rules for.

1.1. If a parameter is part of an object that is not part of the negotiated RPC spec version, that parameter should be removed. The result code should be SUCCESS

1.2. This case is a little tougher. If a value for a parameter is out of range for the negotiated RPC spec version the action should differ based on the mandatory status of that parameter for the specific RPC spec version and if it is or isn't an array.

• If the parameter is non-mandatory, it should be removed and SUCCESS as the result code.
• If the parameter is mandatory and it is not an array of values, the value should be changed to a default value, the result code should be WARNINGS, and the info parameter should include what value was changed.
• If the parameter is mandatory and it is an array of values, the values outside the acceptable range should be removed and SUCCESS as the result code.

2. For a parameter that went from mandatory to non-mandatory, if the param was considered mandatory for the negotiated RPC spec version but the HMI did not provide the param, Core should add the param with a default value, change the result code to WARNINGS, and add an info string that includes what parameters were included this way.

3. If an enum value does not exist in the negotiated RPC spec version while part of an array, that value should be removed from the array, but keeping the array in the RPC. The result code should be SUCCESS

4. If an enum value does not exist in the negotiated RPC spec version while part of an object in an array, that whole object should be removed from the array. The result code should be SUCCESS

Please let me know if this makes sense and potentially feasible.

[theresalech]

The Steering Committee voted to keep this proposal in review, to allow the author time to review and respond to this comment.

[jacobkeeler]

I do recall considering the possibility of using a dummy value to fill out any data which breaks the spec version, though I was worried that it could cause the message to be even more misleading since the data would be faked. I suppose with how specific these cases are, that would be a minor issue compared to what we are solving. Adding the info string also eases some of my concerns.

Another concern I did have was with regards to implementation, as I wasn't certain exactly how to reasonably determine what dummy values to use. After looking into how it would be done for the gps case, the following approaches seem appropriate:

General solution

Dummy values are determined by rules based on parameter type and constraints. This would be much simpler implementation-wise, as we would just need to add a few rules for the validation process to follow.

Potential Rules

• Boolean: false
• Integer/Float:
  
  
  
  • If omitted, 0 (if valid) or minvalue.
    
    
    
    
    
    • ex. altitude: 0, utcDay: 1
    
    
    
  
  
  • If an invalid value is provided, minvalue or maxvalue, whichever is closer to the provided value.
    
    
    
    
    
    • ex. hdop: 15.0 -> 10.0
    
    
    
  
  
• String:
  
  
  
  • If omitted, empty string (if valid) or a minlength string filled with a single character (possibly spaces).
  
  
  • If an invalid value is provided, a truncated or padded version of the invalid value.
  
  
• Enum: First defined enum value
  
  
  
  • ex. compassDirection: "NORTH"
  
  
• Structs: Structure with all mandatory subparameters filled out using other rules for dummy values

Case-by-case

Dummy values would be defined separately for each parameter affected by one of these cases. For the Core implementation, it would be simplest if these values were defined directly in the RPC spec itself (separate from defvalue), though that would expand the scope of this proposal a sizable amount. If using the RPC spec is not possible, these could be hardcoded or stored in a JSON file, but this would not be preferred.

It's also possible we could do a combination of these two solutions as well, using the general rules for most cases and specifying more tailored values when necessary (moreso for String and Enum values).

[jacobkeeler]

Another minor case to consider for array values, suppose we have a mandatory array value with minsize="2", but the array only contains one valid value for the spec version. After the filtering process, the array would be made invalid, so I would suggest padding the array with appropriate dummy values in this case.

Conversely, if an array was larger than maxsize, truncating the array would be necessary.

[theresalech]

It was acknowledged that stripping values as originally proposed will not work for all necessary use cases, so the Steering Committee voted to return this proposal for revisions to incorporate the solutions included in the following comments from the author and Project Maintainer: https://github.com/smartdevicelink/sdl_evolution/issues/1144#issuecomment-848196027, https://github.com/smartdevicelink/sdl_evolution/issues/1144#issuecomment-848921254, and https://github.com/smartdevicelink/sdl_evolution/issues/1144#issuecomment-848935410. The proposal should detail creating a system that applies default/dummy values when applicable and removes values when not, based on the rules included in the comments.

[theresalech]

The author has updated this proposal based on the Steering Committee's agreed upon revisions, and the revised proposal is now in review until June 15, 2021.

The specific items that were updated since the last review can be found in this merged pull request: #1151.

[jacobkeeler]

After some internal discussion, it was determined that there really is no comprehensive solution to this issue that doesn't have any major downsides. This is primarily because Core would be attempting to fix any incorrect data sent from the HMI, so in practice this will need to be fixed on the integrator's end in most cases.

The current plan to approach this issue then would be to encourage integrators to update their minimum RPC version according to what data they support, documentation will be added accordingly to make this process easier for integrators. In the examples given for GPSData, the integrator would set their minVersion to 5.0.0 in their local RPC spec, meaning that apps older than 5.0.0 would not be allowed to register.

The handling of non-mandatory parameters in this proposal is still appropriate, as it does not involve erasing important data or sending fake data. As such, we plan to revise this proposal to only filter non-mandatory parameters, while ignoring any mandatory parameters. This would still be helpful as a failsafe for several cases.

[theresalech]

The Steering Committee voted to return this proposal for the revisions described by the author in this comment.

[theresalech]

The author has updated this proposal based on the Steering Committee's agreed upon revisions, and the revised proposal is now in review until June 29, 2021.

The specific items that were updated since the last review can be found in this merged pull request: #1155.

[theresalech]

The Steering Committee fully agreed to accept this proposal.

[theresalech]

Implementation issue entered: https://github.com/smartdevicelink/sdl_core/issues/3732