[Accepted] SDL 0073 - Adding Metadata Types

[theresalech]

Hello SDL community,

The review of the revised proposal "SDL 0073 - Adding Metadata Types" begins now and runs through July 18, 2017. The original review of "Adding Metadata Types" occurred June 27 through July 5, 2017. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0073-Adding-Metadata-Types.md

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

#208

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]

[joeygrover]

The idea of this change is small, but I believe its impact to be pretty large. The proposed method of adding metadata to a text field through a new struct is ok, but changing the parameter type in an RPC is something none of the projects have handled before. This would introduce a decent amount of workload creating the logic to ensure backwards compatibility and all the testing around it. This would be a breaking change so the RPC spec would have to be increased a major version.

This goes back to the same discussion we had on #189. Our decision for that was as followed:

The Steering Committee voted to defer this proposal in the meeting on May 31, 2017 (2017-05-31). Here is the rationale: : We need a separate proposal on how to handle breaking changes in the RPC Spec.

Using that decision as precedent I believe this proposal would fall under the same decision as #189 and we should have a clear proposal on how to handle breaking RPC changes before we accept any proposals that do so.

For the Mobile_API, we could add new optional parameters for relevant RPCs that define the text field type (ex. "mainField1_type" parameter). This would allow us to keep the existing parameter string type.

Reading through the alternatives there was another solution (5) of introducing a root level param for each text field, eg mainField1_type. This would work around the breaking change aspect. However, the solution itself is very messy and will be difficult to validate each type and field have a matching pair; if a show gets a mainField1_type but no mainField1 would it consider that an error? We would need to make sure we understand exactly how to validate.

We create a new RPC that applications can use to broadcast information. This may increase chatter as multiple RPCs could be sent for the same information (ex. Show and new RPC every time a song changes).

Of all the alternatives I believe introducing a new RPC (3) might be the closest to acceptable for this feature. It would allow OEMs to control which apps can "broadcast" this type of data in the background. It would introduce a challenge of mapping the Show params to the Broadcast fields so we would need to see a clear alignment to move forward. This method could possibly be flushed out enough to map to the "subscribble services" idea and I would be willing to help it get there.

Overall, while the the initial changes look small, the impact is large. I don't believe this proposal adequately addresses the problem it's setting out to solve. The proposal itself would need to be revamped to reflect one of the other alternative solutions or wait until a new proposal is entered and accepted on breaking RPC changes. However, even then I don't believe any of the proposed methods are robust or complete enough; the proposal seems to be a compromise on how to solve a problem for a short term gain and not the long term solution. (Out of the container) I talked about adding "subscribable services" during the discussion around this feature at our first workshop that I still believe is the best way to solve this for the long term. Therefore, I don't believe we should accept this feature as-is or any other feature that isn't robust or complete enough for a long-term strategy.

[Toyota-Sbetts]

@joeygrover Thanks for the feedback. We had quite a bit of discussion ourselves on the exact points you mentioned before we arrived on the current proposal. There were a few reasons we arrived here:

1. We did not want this to be a major version change. We think the current proposal does not have to be a breaking change.
2. We think the current implementation of show is not very expandable and the design is inconsistent with the HMI API. As we add templates with potentially more text fields and objects the current RPC would get very messy as you mention. This change is the first step to get app developers familiar with using the text field objects instead of strings.

The proposed method of adding metadata to a text field through a new struct is ok, but changing the parameter type in an RPC is something none of the projects have handled before. This would introduce a decent amount of workload creating the logic to ensure backwards compatibility and all the testing around it.

Can you please clarify why this would be a decent amount of workload?
Overloading functions and checking variable types has been standard practice for a very long time and we do not consider it difficult to implement or test.
Apps that integrate the new SDL proxy could still use the show command with strings and the proxy should be able to handle this situation and therefore this would not be breaking.
Ultimately, we think we should deprecate the ability to use strings and convert show to take an array of textfield objects like the HMI API but chose not to use this direction at this time as it would be a breaking change.
If the SDLC thinks this is the best direction, we can revise the proposal.

However, even then I don't believe any of the proposed methods are robust or complete enough; the proposal seems to be a compromise on how to solve a problem for a short term gain and not the long term solution. (Out of the container)

Please note this is not the "out of the container" proposal and is not intended to fully solve the problem of sharing information between apps or outside of the current behavior.
It is strictly an enhancement to the existing behavior for OEM HMI's to be able to have more robust and unique implementations and serves as an additional way that OEMs can distinguish themselves.
If an app does not want an HMI to do this, it should simply set the data type to "none" or not set it at all and we have the exact same behavior as today.
For this purpose, we think this proposal does solve that problem and should only be considered in it's own scope.

I talked about adding "subscribable services" during the discussion around this feature at our first workshop that I still believe is the best way to solve this for the long term.

An HMI should not need "subscribable services" to get this type of data from the app but I agree that other apps should.
This use case is out of scope of what we want to do here, but we feel this implementation could be expanded for that use case if that direction is taken by the SDLC.
If you already have some implementation or proposal for this use case that our proposal does not fit into, please let us know so we can work together for a better solution.
But we feel it is not fair to reject a proposal because an SDLC member might have another conflicting proposal that has not yet been introduced or voted on.

Overall, we feel there are many ways to do this and if the SDLC agrees on a different or preferred method we will be happy to revise the proposal.

[joeljfischer]

@Toyota-Sbetts

We did not want this to be a major version change. We think the current proposal does not have to be a breaking change.

Can you clarify if this is adding new parameters, or changing the existing ones? Both Joey and I read it as altering existing parameters.

Adding new parameters

If it's adding new ones, it would be using the same names as the old ones. Not only do some languages (such as Objective-C) not support property (or method) overloading, but these messages are using JSON, and we can't have two keys with the same name! They would absolutely have to be named differently than the existing methods if you are adding new ones.

Changing existing ones

Any change to an existing parameter is, by definition, a major change.

[Toyota-Sbetts]

@joeljfischer Thank you for the information. I think the change we are suggesting is both.

If it's adding new ones, it would be using the same names as the old ones. Not only do some languages (such as Objective-C) not support property (or method) overloading, but these messages are using JSON, and we can't have two keys with the same name! They would absolutely have to be named differently than the existing methods if you are adding new ones.

We would only be adding a new optional parameter to the JSON element between SDL proxy and SDL Core. Currently, my understanding is the proxy is already translating the input string into the JSON format for text fields before sending to SDL Core:

"showStrings" : [
{
    "fieldName" : "mainField1",
    "fieldText" : "Some text"
}

Adding a new optional element "fieldType" : "none" to the above JSON structure should not be a major change and does not duplicate key names.

Any change to an existing parameter is, by definition, a major change.

I assume you're referring to the semantic versioning definition from http://semver.org/.
The definition here reads "MAJOR version when you make incompatible API changes".
If the API between the mobile app and the proxy can accept strings still and no change needs to be made by the app, then we do not believe this is an "incompatible" API change and therefore is not a MAJOR change.

A minor change is defined as a "MINOR version when you add functionality in a backwards-compatible manner" and we would adhere to this.
One straightforward way this could be done as follows (psuedocode):

if (mainField1.type == string)
     convert the string to a textfield object with new optional parameter "fieldType" set as "none"
     This is basically the current behavior anyway.
else if (mainField1.type = textfield object)
     No conversion needed, pass through the object to convert to JSON

This is obviously just one simple solution and there are many other more robust solutions like utilizing protocols.

To be honest, we do not understand why the mobile API was designed differently in the first place and feel this is one step towards correcting this mistake and having a robust and expandable function.

[brandon-salahat-tm]

To build on Scott's comment, Objective-C does offer a fairly elegant solution to migrating this interface via Protocol/Functional Oriented Programming. Please find a simple implementation below.

SDLMainFieldProtocol.h

#import <Foundation/Foundation.h>
@class SDLMainField;

@protocol SDLMainFieldProtocol <NSObject>
- (SDLMainField *)mainFieldObject;
@end

NSString+SDLMainField.h

#import <Foundation/Foundation.h>
#import "SDLMainFieldProtocol.h"

@interface NSString (SDLMainField) <SDLMainFieldProtocol>

@end

NSString+SDLMainField.m

@implementation NSString (SDLMainField)

- (SDLMainField *)mainFieldObject {
    return [[SDLMainField alloc] initWithText:self withType:0]; // 0 -> No Type in enum
}

@end

SDLMainField.h

#import <Foundation/Foundation.h>
#import "SDLMainFieldProtocol.h"

@interface SDLMainField : NSObject <SDLMainFieldProtocol, NSCopying>
@property (nonatomic, copy) NSString *text;
@property (nonatomic, assign) int type; //would be an enum

- (instancetype)initWithText:(NSString *)text withType:(int)type;

@end

SDLMainField.m

#import "SDLMainField.h"

@implementation SDLMainField

- (instancetype)initWithText:(NSString *)text withType:(int)type {
    if (self = [super init]) {
        self.text = text;
        self.type = type;
    }
    
    return self;
}

#pragma mark NSCopying
- (id)copyWithZone:(NSZone *) zone
{
    SDLMainField *copy = [[SDLMainField allocWithZone: zone] init];
    
    copy.text = [self.text copy];
    copy.type = self.type;
    
    return copy;
}

#pragma mark SDLMainFieldProtocol
- (SDLMainField *)mainFieldObject {
    return self;
}

@end

ViewController.h

#import <UIKit/UIKit.h>
#import "SDLMainFieldProtocol.h"

@interface ViewController : UIViewController
@property (nonatomic, copy) NSObject<SDLMainFieldProtocol> *mainField;

@end

ViewController.m

#import "ViewController.h"
#import "NSString+SDLMainField.h"
#import "SDLMainField.h"

@interface ViewController ()

@end

@implementation ViewController

- (void)viewDidLoad {
    [super viewDidLoad];
    // Do any additional setup after loading the view, typically from a nib.
    self.mainField = @"String Test";
    NSLog(@"mainField is %@", self.mainField.debugDescription);
    NSLog(@"mainField value is %@", [[self.mainField mainFieldObject] debugDescription]);
    
    self.mainField = [[SDLMainField alloc] initWithText:@"Object Test" withType:1];
    NSLog(@"mainField is %@", self.mainField.debugDescription);
    NSLog(@"mainField value is %@", [[self.mainField mainFieldObject] debugDescription]);
}



- (void)didReceiveMemoryWarning {
    [super didReceiveMemoryWarning];
    // Dispose of any resources that can be recreated.
}


@end

Additionally we would want to tag the string compatibility interface (NSString+SDLMainField in this example) with XCode's deprecated tag, with an explanation to implementers that it will be phased out in the next major release (or whenever we decide to deprecate it completely). I believe this is a sane way to begin migration of the interface without introducing a breaking change. The app developer's would not notice any difference in the interface, besides our deprecation warning.

I believe Android could support a similar or equivalent solution.

I can provide the entire XCode project I used to rough this out, if Livio would like to further review it.

[joeljfischer]

@Toyota-Sbetts

To be honest, we do not understand why the mobile API was designed differently in the first place and feel this is one step towards correcting this mistake and having a robust and expandable function.

I will be the first to grant that the mobile API was designed poorly to begin with, but we have to now work with what we have and we can't break backward compatibility without doing so intentionally. I'm convinced that this proposal does make major version changes, unless I'm fundamentally misreading or misunderstanding something.

I'm not sure what this is meant to convey:

"showStrings" : [
{
    "fieldName" : "mainField1",
    "fieldText" : "Some text"
}

There does not seem to be any such field as showStrings in the SDL mobile API.

If we're talking about the Show RPC here, there is no type on mainField1. It's type is String. If we change that type, it's backward incompatible and a major change. The TextFieldName is used only in DisplayCapabilities. You are not simply adding an optional parameter here. You are fundamentally changing the type of the Show request's parameters. That is a major change, even if the proxy can work around it to make it backward compatible internally moving forward.

The existing JSON would look like this:

{
  "mainField1": "Some string",
  "mainField2": "Some other string"
}

Your proposal would change it to this:

{
  "mainField1": {
    "fieldName": "mainField1",
    "fieldText": "Some String",
    "fieldType": "mediaTitle"
  }
}

Let me know if I'm misreading this, but this is absolutely a breaking change. We are modifying an API, which is a backward incompatible change. Even if we work around it in the libraries for older head units, that would break people's code, and be a major change in the libraries. Plus then we also have to account for head units connecting to older libraries.

You provide the following pseudocode:

if (mainField1.type == string)
     convert the string to a textfield object with new optional parameter "fieldType" set as "none"
     This is basically the current behavior anyway.
else if (mainField1.type = textfield object)
     No conversion needed, pass through the object to convert to JSON

However I'm failing to see the parallels here. convert the string to a textfield object with new optional parameter "fieldType" set as "none". This is basically the current behavior anyway. This is not the current behavior. The current behavior is to set a string directly, not a new struct field. Even if we could overload, this is still a breaking change to the API, the API was fundamentally one way, then we modified it, not merely adding stuff, which is a major version change to the API. Even if we could overload the methods, that would be highly confusing to library users and not reflective of the API as it exists. We would be abstracting in an odd and confusing way.

@Toyota-BSalahat
I'm not precisely sure what you're attempting to do here, as MainField isn't a type in SDL. If I am understanding right, however, this is a breaking change for the iOS library as the API would have to be modified and current integrations rewritten.

[brandon-salahat-tm]

I'm not precisely sure what you're attempting to do here, as MainField isn't a type in SDL. If I am understanding right, however, this is a breaking change for the iOS library as the API would have to be modified and current integrations rewritten.

@joeljfischer The code I posted was an example of how this could be done specifically without breaking the Proxy API (which is what integrations care about). SDLMainField is not currently in the proxy, but is an example of what could be added.

As I stated, I can send you the XCode project for you to review if it is not clear from reading the code snippets in my previous comment.

[joeljfischer]

@toyota-bsalahat I think I'd need a slightly larger example with real types to understand what you're trying to do here. I get the code, but I'm not sure how it fits with what we're talking about. In any case, the API isn't in the library, it's in the XML. Versioning an API is pretty easy: if you remove or alter an existing API (outside of adding optional parameters), it's a major change. If you add a new API, alter an existing API by adding optional parameters, or deprecate an existing API, it's a minor change. This alters the type of existing APIs, therefore it is a major version change. Whether or not we can find hacks and workarounds in the libraries is immaterial to that discussion.

Now, if we can find workarounds in the libraries, that is helpful, but I haven't seen how that could happen without some serious abstractions. We have never fundamentally altered the type of an existing API, and we'd need some hard guidelines of how that's to be done.

[brandon-salahat-tm]

@joeljfischer can I email you the existing example? I think it will make sense to you if you look at it.

It is a basic implementation utilizing protocol oriented programming and iOS categories. I could carry these changes into the iOS proxy to show you, but I do not think it will aid in your understanding of the example, and would take some time.

I think we have two threads going about the XML changes, and the actual Proxy changes to support it. My example is a demonstration of how we could migrate the Proxy interface without breaking it, or affecting current implementations.

Additionally, I could schedule a call today and we could talk through my example if that would be helpful.

[joeljfischer]

@toyota-bsalahat I sent you a slack message.

[Toyota-Sbetts]

@joeljfischer I'm sorry, I think maybe there is some misunderstanding of my comment.

There does not seem to be any such field as showStrings in the SDL mobile API.

That's because this is the field received by SDL Core and passed to the HMI and is not the mobile API.
Here is what SDL Core translates the show command to pulled straight from a SDL Core log:

{
   "id" : 147,
   "jsonrpc" : "2.0",
   "method" : "UI.Show",
   "params" : {
      "appID" : 123456789,
      "graphic" : {
         "imageType" : "DYNAMIC",
         "value" : "/000000002_a52c74c247dbae82164a73117e278a5f2b91fbe9b7caeed4429f120f81da686f/possibilities.jpg"
      },
      "showStrings" : [
         {
            "fieldName" : "mainField1",
            "fieldText" : "Some text"
         },
         {
            "fieldName" : "mainField2",
            "fieldText" : "Some text"
         },
         {
            "fieldName" : "mainField3",
            "fieldText" : "Some text"
         },
         {
            "fieldName" : "mainField4",
            "fieldText" : "Some text"
         },
         {
            "fieldName" : "mediaTrack",
            "fieldText" : "Some text"
         }
      ],

Therefore, the only new parameter we are suggesting to add is to the text field struct which would turn each of those elements to the following:

 {
            "fieldName" : "mainField1",
            "fieldText" : "Some text",
            "fieldType" : "none"
 }

So, at some point the "string" get translated to this proper textfield type.
Whether that is in SDL proxy or SDL Core, we think this change can be handled gracefully without any breaking changes.
As Brandon mentioned, we're happy to have a call with Livio to clarify this in more detail.

We have never fundamentally altered the type of an existing API, and we'd need some hard guidelines of how that's to be done.

We have seen multiple proposals deferred for similar reasons (Joey mentioned #189).
Can you clarify for me who is responsible for creating and defining these guidelines?
Is a member of the SDLC supposed to create a proposal to define this before we can make any proposals of this kind?
We cannot keep rejecting or deferring proposals without any clear path on how to move forward with them.

[joeljfischer]

@Toyota-Sbetts I don't think the HMI_API alterations concern us too much unless @joeygrover has any feedback on that. It's the MOBILE_API changes that concern us.

I believe the proposal you referenced is the only one which was discussed in this capacity until now. I believe the SDLC as a group would have to define these guidelines. Livio would be willing to help do so. The reason we haven't yet defined such guidelines is that there have not been any proposals that have been desired enough by the SDLC to make a breaking change.

[Toyota-Sbetts]

@joeljfischer I think I understand where there is misunderstanding. Currently SDL Core is translating the Mobile API to the HMI API. Our understanding was that Mobile API defined the app to proxy interface and not the proxy to core interface.
SDL Core takes the following from the mobile API:

{
    "mainField1":"Some text",
    "mediaTrack":"1\/4",
    "mainField2":"Some text"
}

And translates it to the HMI API:

"showStrings" : [
         {
            "fieldName" : "mainField1",
            "fieldText" : "Some text"
         },
         {
            "fieldName" : "mainField2",
            "fieldText" : "Some text"
         },
         {
            "fieldName" : "mediaTrack",
            "fieldText" : "1\/4"
         }

I feel this leaves us with some options:

1. We follow the current proposal which is a breaking change to the RPC spec (Mobile_API) and define how breaking changes should be handled. This is still not a bad direction, but will require more time.
2. We can follow one of the other alternatives mentioned in the proposal that are not breaking changes.
3. We think another alternative is the RPC spec can add a new JSON element array "metadata" that contains the enumeration of metadata types contained in the RPC and which text field is set in it. All text fields which are not set to a metadata type are assumed to be "none". Old versions of SDL Core would ignore this element and it should not be a breaking change. For example:

{
    "mainField1":"Some text",
    "mediaTrack":"1\/4",
    "mainField2":"Some text",
    "metadata":[
        "artist":"mainField1",
        "album":"mainField2",
        :
]}

Overall, it's likely that we will need to revise the implementation details in this proposal but overall we still think the purpose of this proposal and it's general solution is valid.
Additionally, where do we define the application to proxy interface?

The reason we haven't yet defined such guidelines is that there have not been any proposals that have been desired enough by the SDLC to make a breaking change.

I'm not sure this is how we should go about this.
This seems like a very important topic that we should address before these proposals are entered not after.

Edit: Updated comment based on misunderstanding of Mobile API.

[joeljfischer]

I think option 3 is doable, it's...inelegant, but finding an elegant solution may not be possible.

Additionally, where do we define the application to proxy interface?

The proxy is the application to proxy interface. Or, more precisely, the proxy is an abstracted interface for the application to commuicate with Core.

[Toyota-Sbetts]

@joeljfischer Thanks for you feedback, I think we're more or less getting on the same page now. We agree option 3 is not the most elegant solution but without making a breaking change it may be the best way.

As I mentioned, we expected discussion on which is the best way to handle adding these parameters because there is no one clear answer and we'll revise the proposal as necessary based on the SDLC's preferred method.

The proxy is the application to proxy interface. Or, more precisely, the proxy is an abstracted interface for the application to commuicate with Core.

What I mean is does a defined specification for the abstracted interface exist or should this interface match the MOBILE_API 1:1 or is it up to each individual proxy?

[theresalech]

Given the discussion that has taken place on this issue, the Steering Committee has voted to defer this proposal and hold a separate workshop to determine appropriate revisions. The workshop will take place on July 10, 2017 (2017-07-10), so voting on this issue will take place after that date.

[theresalech]

This proposal has been revised to include the feedback received during the July 10, 2017 workshop. It is now in review until July 18, 2017 (2017-07-18).

[joeljfischer]

I believe this proposal should be accepted.

[theresalech]

Now that the requested revisions have been incorporated, the Steering Committee has agreed to accept this proposal.

[theresalech]

Issues Entered:
Android
iOS
Core
RPC