[Accepted] SDL 0163 - Make spaceAvailable field non-mandatory

[theresalech]

Hello SDL community,

The review of "Make spaceAvailable field non-mandatory" begins now and runs through May 1, 2018. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0163-make-spaceavailable-field-non-mandatory.md

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

#477

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
theresa@livio.io

[joeljfischer]

It is not entirely true that no validation is done on messages received from Core by the Proxies. Managers, developer code, and nullability (in the iOS library) all could indicate requirements for mandatory properties and major changes like this could cause breakage. With that said, I don't think this particular change will cause any issues with mobile code due to Core not properly handling the case. Nevertheless, this would have to be a major version change, which is not specified in the proposal.

[ghost]

I think .spaceAvailable follows a slightly different logic than etra parmaters in other response messages. I think in case there's not enough space for the requested file the result code should be OUT_OF_MEMORY but the app wouldn't know how much space is left if the parameter is omitted.

[joeljfischer]

@kshala-ford To be clear, the spaceAvailable field is not currently sent in error messages even though it is mandatory.

[jacobkeeler]

@kshala-ford I have no problem with Core sending spaceAvailable in that specific OUT_OF_MEMORY case, but the current setup requires that this parameter be sent even in cases where it isn't relevant, such as in the case of an APPLICATION_NOT_REGISTERED response. It is my argument that it shouldn't be mandatory in all cases (and, as Joel reiterated, SDL Core does not currently respect the fact that it currently mandatory).

[jacobkeeler]

@joeljfischer As mentioned in the proposal, my understanding was that there was no validation done on outgoing messages from Core. This was my justification for considering this a documentation change, which would only result in a patch version change, rather than a functional one. While you have stated that this is not entirely the case, I've not been able to find any issues with the proxies that have been reported because of this difference between the spec and actual behavior (which has existed since the sdl_core GitHub repo was created).

It seems to me that a major version change would only be required if this could actually break someone's implementation. We already know it doesn't, because Core already doesn't treat this parameter as mandatory, meaning that any implementations that rely on this have been broken since the creation of the project. In addition, no functional changes in the proxies or SDL Core would be required to support this change (to my knowledge). With these considerations, It doesn't appear that this could be a breaking change in any way.

All this said, if the SDLC doesn't agree with my reasoning, this change can be delayed until the next major release of Core. It just needs to be noted that this means that this discrepancy will exist in SDL Core until that release in that case.

[ghost]

I'm sorry. I guess I was just noting down thoughts comparing this param with those that should only occur on success responses... Nevermind...

I agree to @jacobkeeler last comment. I don't believe it's a major version change. Not even a version change. I guess this parameter was considered as optional when it was added but it wasn't properly tracked in the spec. Could be a legacy issue from the time before SDL... So the following sentense in the proposal makes sense.

The mandatory field was later made necessary for every field, and this field was changed accordingly at that time. As such, it seems that this was likely done on accident originally.

That said the attribute should be set to mandatory="false" without any code change or versioning to simply correct the mistake.

[joeljfischer]

I don't believe that intent matters, nor mitigating factors. What matters is that our versioning describes the changes taking place with a strict set of rules in order to provide predictability. The rules state that changing this flag is a major version change. We can mitigate it however we want, but according to the letter of semantic versioning, this is a major version change. If we want to break those rules in this case, that's the decision the SDLC would have to make.

See:

• https://semver.org/#if-even-the-tiniest-backwards-incompatible-changes-to-the-public-api-require-a-major-version-bump-wont-i-end-up-at-version-4200-very-rapidly
• https://semver.org/#what-do-i-do-if-i-accidentally-release-a-backwards-incompatible-change-as-a-minor-version

[jacobkeeler]

@joeljfischer To be entirely clear, I do not think that this is a backwards-incompatible change, for the reasons I've stated above (though I used the term "breaking change" in that case). As the documentation that you've linked says, a major version change is necessary only when backwards-incompatible changes are introduced.

Again, the SDLC might not agree with my arguments, but if they do, they would not be breaking the rules of semantic versioning if this change is introduced before the next major release.

Do we have any documentation stating that changing this flag would be considered a backwards-incompatible change (or even a decision on another proposal that could be used as precedent)? If so, that would provide a good argument against mine.

[joeljfischer]

@jacobkeeler See: #189

[joeygrover]

I believe what @jacobkeeler is reasoning is that because the coded projects currently don't expect the parameter to be mandatory it could be looked at as a mistake in the RPC documentation.

However, since we offer the spec as its own project this will be a breaking change. We have software projects that are based on that spec, but those projects have incorrectly implemented the spec themselves. So while changing the spec to reflect the software projects seems like not a breaking change, the fact that someone could theoretically have written their own proxies or core implementation based on the RPC spec makes it not possible for us to do without considering it a breaking change.

[jacobkeeler]

After talking with Joey separately on this matter, he has convinced me that this change would indeed be backwards-incompatible in the RPC spec. While it would not cause issues in any of the projects provided by the SDLC, any developer using the RPC spec solely as the basis for their SDL implementation (i.e. they have their own custom Core, HMI, and proxies) could theoretically be broken by this change.

If the SDLC is willing to risk breaking such custom implementations, then this could still be introduced before the next major release of the RPC spec. However, I now agree that this would require the SDLC making a specific exception to the current versioning process.

[ghost]

At the end we are talking about a simple bug, right? The question is where's the bug located?

1. The bug is in the RPC spec and Core is correct. Fix the bug by changing mandatory="true" to mandatory="false"

2. The bug is not in the RPC spec but in Core. Fix the bug using versioning and set mandatory="false" in the next major version.

It sounds like the project maintainers prefer to move forward with the latter one with the reason that the first could hypothetically break with potential implementations other than the official ones. Please correct me if I'm mistaken.

If we move forward with the second proposal it'll be a mistake we will keep carrying forever. That might not be that much of a big deal as it's simply a few more lines of XML in the mobile API.

I'm struggling with the argument of a hypothetically break with potential implementations because with SYNC3 we have many cars on the road which treat that parameter as optional. Moving forward with 2. would officially call the behavior as a bug in production head units. That's why I believe it's a higher score than hypothetical non-official implementations.

I'm very sure it's a bug/mistake in the XML file. Treating this mistake as the correct way would define today's behavior as a bug... I don't think that's what we should go forward with.

[jacobkeeler]

@kshala-ford We would be going with option 1., the change just might need to be delayed until the next major release of the spec unless the SDLC decides that breaking these potential implementations is an acceptable risk.

[theresalech]

The Steering Committee voted to accept this proposal. It was agreed that this would be a breaking change for the RPC Spec alone, since it's been released as its own project, and this is a public API change. It was also confirmed that while previous versions of Core were not up to the correct RPC Spec, the proxies were able to handle this case already and no change is needed in Core.

[theresalech]

Issue Entered: [SDL 0163] Make spaceAvailable field non-mandatory