[SDL-0234] Proxy Library RPC Generation #1273

vladmu · 2020-02-12T19:55:27Z

Fixes #1090

This PR is [ready] for review.

Risk

This PR makes [no] API changes.

Testing Plan

I have verified that I have not introduced new warnings in this PR
I have run the unit tests with this PR
I have tested Android, Java SE, and Java EE

Unit Tests

Covered by unit tests and PyLint

Summary

According to [SDL-0234] Proxy Library RPC Generation proposal, this PR adds the Python generator script based on XML Parser from smartdevicelink/rpc_spec linked to the repository as Git submodule

Changelog

Enhancements

Added utils/generator/rpc_spec submodule pointed to develop branch of smartdevicelink/rpc_spec repository
Created Proxy RPC Generator based on Python 3.5 and linked with parser from the utils/generator/rpc_spec submodule
Added README.md with usage description and transformation rules for Proxy RPC Generator

CLA

I have signed the CLA

vladmu · 2020-02-14T17:00:39Z

@theresalech @joeljfischer @joeygrover
Based on this conversation #1090 (comment) we'd like to notify here that we have found missing parameters in 3 enums that exist in XML but don't exist in the current code:

FunctionID.RESERVED
AppInterfaceUnregisteredReason.UNSUPPORTED_HMI_RESOURCE
Result.CHAR_LIMIT_EXCEEDED

This leads to the failed result when we try to check the generated code with existing unit tests. Because they are part of the current MOBILE API specification, it seems missing parameters should be included in the current RPC code and unit tests should be adjusted.

kshala-ford

Found a few items but nothing important. I was looking for an exception of GenericResponse in the README but couldn't find anything.

utils/generator/README.md

Co-Authored-By: Kujtim Shala <kshala@ford.com>

…ord itself.

vladmu · 2020-02-17T16:57:04Z

@kshala-ford exception of *Response added, all suggested changes accepted, questions answered. Please take a look.

kshala-ford · 2020-02-17T17:32:46Z

@theresalech I approve this pull request.

theresalech · 2020-02-17T18:04:13Z

@vladmu, if this is ready for Livio review, can you please update the PR description from This PR is **[not ready]** for review. to This PR is **[ready]** for review.? Thanks!

vladmu · 2020-02-17T19:31:56Z

@theresalech done, the PR is ready for Livio review. Thank you!

bilal-alsharifi · 2020-02-18T15:04:02Z

Thanks, @vladmu for implementing the PR. I am just wondering if there is a specific reason for making the files in templates/scripts/ have the .groovy extension ? can we make them use .java instead? because they are snippets from java files.

ghost · 2020-02-18T15:08:26Z

Hello, @bilal-alsharifi I completely agree with your point. Will do this now.

bilal-alsharifi · 2020-02-19T21:07:02Z

Thanks, @ksologubov for making the changes.

.gitignore

utils/generator/README.md

bilal-alsharifi · 2020-02-19T21:07:33Z

As it was discussed in the iOS issue, no mappings for corner cases should be specified in the generator. Generic rules are fine (like replacing “sync” with “sdl” for example). The script should use the RPC specs without any case by case handling. Even if that would generate a different code than what the Java Suite currently has. Handling RPCs on case by case basis will make the scripts hard to maintain. Some examples of how things in the current mappings can be removed are:

Enum values can always use the name attribute and completely ignore the internal_name. This, I believe, will get rid of the use for the kind (simple, custom, complex) attribute that is specified for the enums in mapping.json. More generic rules can be added to make the variable accepted by the compiler like always adding _ if the names start with a number.
Float can always be used if the RPC spec says so. It doesn’t need to be replaced with Double.
Because FunctionID is very different than any other enum, it shouldn’t be generated by the generator. We can always manually modify it.
No need to add any methods that do not come directly from the spec (examples: format() ...)
Imports shouldn’t be specified as corner cases. I think that would be possible after removing the corner cases methods as mentioned previously.

The previous list just gives some examples and it is not comprehensive. But the important part is to avoid making special cases for each item.

kshala-ford · 2020-02-20T10:53:34Z

@theresalech Please add an agenda item about SDL-0234 implementation to the next steering committee meeting. The requested changes here and the discussion in sdl_ios are not compliant to the accepted proposal.

…ration # Conflicts: # generator/rpc_spec

ghost · 2020-04-16T09:28:29Z

Because of the getFunctionID() issue that we mentioned in the previous comment and some other similar name conflicting issues in iOS, we added a list of reserved keywords that RPC Generators in all platforms (iOS, Java Suite, JavaScript Suite) should avoid using by following these rules:

If the generator needs to create a variable or method and the name was one of the names in the reserved keyword, the generator should append the suffix Param to it. For example getFunctionID > getFunctionIDParam

Any empty line in the reserved keywords file should be ignored

Any line in the reserved keywords file that starts with one or more # symbols then space should be considered a comment and ignored (like #<space> or ##<space> or ###<space> …). Note that some reserved keywords have a # without a <space> so it can't just check for #.

Reserved keywords matching should be case insensitive

Although the reserved keywords list has a section for each platform, the generator for each platform should avoid using any of the keywords whether it belongs to that platform or another one.

The reserved keywords list is part of the rpc_sepc repo so all generators can access it. Please look at this PR for more info about the reserved keywords.

@bilal-alsharifi Keywords logic was applied to PR. Could you please check from your side?

bilal-alsharifi · 2020-04-16T16:22:52Z

Thanks, @ksologubov for making the changes. It looks like at some point in the recent updates to the PR. The script stopped setting the default OUTPUT_DIRECTORY automatically. So when I run this command now:

python3 ./generator.py -y

I get this message:

usage: generator.py [-h] [-v] [-xml SOURCE_XML] [-xsd SOURCE_XSD] -d
                    OUTPUT_DIRECTORY [-t [TEMPLATES_DIRECTORY]]
                    [-r REGEX_PATTERN] [--verbose] [-e] [-s] [-m] [-y] [-n]
generator.py: error: the following arguments are required: -d/--output-directory

It should always default to the correct OUTPUT_DIRECTORY if no other directory is specified using the -d argument

bilal-alsharifi · 2020-04-16T20:11:18Z

@ksologubov While I am testing the new changes, I found that in a lot of cases, the generator appends "Param" when it shouldn't. Here are a few examples:

Example 1:

public class HapticRect extends RPCStruct {
    public static final String KEY_ID_PARAM = "idParam";
}

Note: The JSON values should never change:

Example 2:

public GetAppServiceDataResponse(@NonNull Boolean successParam, @NonNull EnumSubset resultCodeParam) {
    this();
    setSuccessParam(successParam);
    setResultCodeParam(resultCodeParam);
}

Note: These params/setters shouldn't have "Param" since they are calling methods in predefined base classes. Also I am not sure why result is of type EnumSubset instead of Result

Example 3:

public class CancelInteraction extends RPCRequest {
    public static final String KEY_FUNCTION_IDPARAM = "functionID";
}

Note: The key should be KEY_FUNCTION_ID_PARAM not KEY_FUNCTION_IDPARAM

The aforementioned snippets are just samples and not a comprehensive list. I recommend using a directory comparison tool (I use Meld) to compare the RPC output before the reserved keywords changes and after them to confirm that only cases that actually need to be changed have changed.

ghost · 2020-04-21T17:12:37Z

Example 2:

public GetAppServiceDataResponse(@NonNull Boolean successParam, @NonNull EnumSubset resultCodeParam) {
    this();
    setSuccessParam(successParam);
    setResultCodeParam(resultCodeParam);
}

* Note: These params/setters shouldn't have "Param" since they are calling methods in predefined base classes. Also I am not sure why result is of type `EnumSubset` instead of `Result`

@bilal-alsharifi Bilal, get/setSuccess and get/setResultCode both explicitly specified in keywords list. And the same is for success and resultCode params. Thats why Param is appended in all cases here. From what you said it should not be there in keywords list. What should we do in this case? I believe better to remove them from keywords list.

bilal-alsharifi · 2020-04-21T17:49:18Z

I understand the confusion. In that example, they are calling APIs from the base class. The reserved keywords rules apply to APIs in the subclasses (RPCs) but don't apply to APIs in base classes (RPCMessage, RPCStruct, RPCNotification, RPCRequest, RPCResponse). The setSuccess(), setResultCode() are APIs in the base class so they should not be affected by the reserved keywords rules.

public GetAppServiceDataResponse(@NonNull Boolean success, @NonNull Result resultCode){
	this();
	super.setSuccess(success);
	super.setResultCode(resultCode);
}

ghost · 2020-04-22T09:13:50Z

@bilal-alsharifi but according to smartdevicelink/rpc_spec#232

If the generator needs to create a variable or method and the name was one of the names in the reserved keyword, the generator should append the suffix Param to it. For example getFunctionID > getFunctionIDParam

In this case generator working exactly as proposal says by replacing setSuccess keyword in method name with setSuccessParam and so on. "super" was not used in original library implementation so I'm not sure why you provided it above. Please clarify why setSuccess/setResultCode/success/resultCode should not be removed from keyword list if it should not be replaced. I don't see how it's used in any other cases except calling methods from RPCResponse base class from response type functions.

From my side I can exclude replacement of setSuccess/setResultCode/success/resultCode keywords in response functions which will also fix this case. Please approve this solution.

bilal-alsharifi · 2020-04-22T15:03:25Z

@ksologubov I think you are right. I removed info, success, resultCode from reserved keywords. I will do another review and let you know if there is any more feedback.

bilal-alsharifi · 2020-04-22T16:13:09Z

@ksologubov I see set/getFunctionId() inside the class CancelInteraction are still not affected by the reserved keywords. They should be set/getFunctionIdParam() as they are the main reason for adding the reserved words.

bilal-alsharifi · 2020-04-22T16:17:43Z

Can we pull the latest changes for the rpc_spec submodule after removing the response keywords?
Also, changing the submodule init command in the README to be:

git submodule update --init --recursive --remote

will be helpful to guarantee that the latest changes for the submodule are always pulled.

bilal-alsharifi · 2020-04-22T21:57:41Z

generator/transformers/common_producer.py

+            return re.sub(r'^([sS])ync(.+)$', r'\1dl\2', name)
+        return name
+
+    def replace_keywords(self, name: str = '') -> str:


The comparison in this method seems to be case sensitive. It should be insensitive.

@bilal-alsharifi restored case insensitive matching, and also fixed functionID parameter replacement, thanks

ghost · 2020-04-23T09:56:24Z

@bilal-alsharifi thanks for removing extra keywords. All above problems was fixed, and suggestions was applied, I checked classes diff with Meld and they looking good from my side, could you please check from your side?

bilal-alsharifi · 2020-04-23T15:04:38Z

@ksologubov Thanks for making the changes. I have reviewed and tested the PR. It looks good now.

ghost · 2020-04-23T15:30:32Z

@bilal-alsharifi thank you very much for reviews

[SDL-0234] Proxy Library RPC Generation

5c53a23

vladmu requested review from kshala-ford and mrapitis February 12, 2020 19:55

Kostiantyn Sologubov added 4 commits February 13, 2020 20:55

fixing android tests

7622ce3

fixing android tests

f008ba5

fixed android tests

b1d4c4e

Removed customization for new enum elemets

27c7066

kshala-ford suggested changes Feb 17, 2020

View reviewed changes

vladmu and others added 3 commits February 17, 2020 18:39

Apply suggestions from code review

53998b0

Co-Authored-By: Kujtim Shala <kshala@ford.com>

More wide rule for responses

ffb3474

Added notice regarding Response functions ended with the 'Response' w…

a5a86d1

…ord itself.

kshala-ford approved these changes Feb 17, 2020

View reviewed changes

mrapitis approved these changes Feb 17, 2020

View reviewed changes

Kostiantyn Sologubov added 2 commits February 18, 2020 17:19

renaming groovy files to java

8ebf010

renaming groovy files to java

d81628f

bilal-alsharifi reviewed Feb 19, 2020

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

utils/generator/README.md Outdated Show resolved Hide resolved

utils/generator/README.md Outdated Show resolved Hide resolved

Kostiantyn Sologubov added 4 commits February 20, 2020 12:55

replacing sync in names

a15ae81

removing corner cases and mapping

95275d6

replacing sync in names

72f1aa5

replacing sync in names fix

7e1e227

Kostiantyn Sologubov added 6 commits April 8, 2020 19:48

Return to stable version of github action checkout

18b4c49

moving generators to repository root

e3c4b4f

fixed enum tests after previous changes

f509c13

fixed path in github action file

56408fc

fixed tests

7a4ff35

Merge branch 'SDL-0234-keywords' into SDL-0234-Proxy-Library-RPC-Gene…

0e7012d

…ration # Conflicts: # generator/rpc_spec

theresalech mentioned this pull request Apr 15, 2020

[SDL 0234] Proxy Library RPC Generation #1090

Closed

Merge branch 'develop' into SDL-0234-Proxy-Library-RPC-Generation

be7f985

Kostiantyn Sologubov added 2 commits April 21, 2020 11:21

fixed default path

445d30b

fixed struct element naming

b7b175f

removed caseless keyword matching

a9a3d51

Kostiantyn Sologubov added 2 commits April 22, 2020 22:19

updated rpc_spec to latest

ac1591d

readme update

ab1c57a

bilal-alsharifi reviewed Apr 22, 2020

View reviewed changes

Kostiantyn Sologubov added 2 commits April 23, 2020 12:00

Revert "removed caseless keyword matching"

48ef926

correct key name processing

350709d

bilal-alsharifi approved these changes Apr 23, 2020

View reviewed changes

bilal-alsharifi merged commit 988784f into smartdevicelink:develop Apr 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDL-0234] Proxy Library RPC Generation #1273

[SDL-0234] Proxy Library RPC Generation #1273

vladmu commented Feb 12, 2020 •

edited

vladmu commented Feb 14, 2020

kshala-ford left a comment

vladmu commented Feb 17, 2020

kshala-ford commented Feb 17, 2020

theresalech commented Feb 17, 2020

vladmu commented Feb 17, 2020

bilal-alsharifi commented Feb 18, 2020

ghost commented Feb 18, 2020

bilal-alsharifi commented Feb 19, 2020

bilal-alsharifi commented Feb 19, 2020 •

edited

kshala-ford commented Feb 20, 2020

ghost commented Apr 16, 2020

bilal-alsharifi commented Apr 16, 2020

bilal-alsharifi commented Apr 16, 2020

ghost commented Apr 21, 2020 •

edited by ghost

Example 2:

bilal-alsharifi commented Apr 21, 2020 •

edited

ghost commented Apr 22, 2020 •

edited by ghost

bilal-alsharifi commented Apr 22, 2020

bilal-alsharifi commented Apr 22, 2020

bilal-alsharifi commented Apr 22, 2020 •

edited

bilal-alsharifi Apr 22, 2020

ghost Apr 23, 2020

ghost commented Apr 23, 2020

bilal-alsharifi commented Apr 23, 2020

ghost commented Apr 23, 2020

[SDL-0234] Proxy Library RPC Generation #1273

[SDL-0234] Proxy Library RPC Generation #1273

Conversation

vladmu commented Feb 12, 2020 • edited

Risk

Testing Plan

Unit Tests

Summary

Changelog

Enhancements

CLA

vladmu commented Feb 14, 2020

kshala-ford left a comment

Choose a reason for hiding this comment

vladmu commented Feb 17, 2020

kshala-ford commented Feb 17, 2020

theresalech commented Feb 17, 2020

vladmu commented Feb 17, 2020

bilal-alsharifi commented Feb 18, 2020

ghost commented Feb 18, 2020

bilal-alsharifi commented Feb 19, 2020

bilal-alsharifi commented Feb 19, 2020 • edited

kshala-ford commented Feb 20, 2020

ghost commented Apr 16, 2020

bilal-alsharifi commented Apr 16, 2020

bilal-alsharifi commented Apr 16, 2020

Example 1:

Example 2:

Example 3:

ghost commented Apr 21, 2020 • edited by ghost

Example 2:

bilal-alsharifi commented Apr 21, 2020 • edited

ghost commented Apr 22, 2020 • edited by ghost

bilal-alsharifi commented Apr 22, 2020

bilal-alsharifi commented Apr 22, 2020

bilal-alsharifi commented Apr 22, 2020 • edited

bilal-alsharifi Apr 22, 2020

Choose a reason for hiding this comment

ghost Apr 23, 2020

Choose a reason for hiding this comment

ghost commented Apr 23, 2020

bilal-alsharifi commented Apr 23, 2020

ghost commented Apr 23, 2020

vladmu commented Feb 12, 2020 •

edited

bilal-alsharifi commented Feb 19, 2020 •

edited

ghost commented Apr 21, 2020 •

edited by ghost

bilal-alsharifi commented Apr 21, 2020 •

edited

ghost commented Apr 22, 2020 •

edited by ghost

bilal-alsharifi commented Apr 22, 2020 •

edited