[SDL 0234] Proxy Library RPC Generation

[theresalech]

Proposal: SDL 0234 - Proxy Library RPC Generation

This proposal adds automatic RPC generation for iOS and Java from the XML spec via a python script.

smartdevicelink/sdl_evolution#741

Steering Committee Decision:

The Steering Committee voted to approve this proposal with the following revision: there will be parsing code in each repository, and the rpc spec will be a submodule that pins which rpc spec will be used in each version of the library.

Revisions were made on 2019-06-10

[vladmu]

Greetings library maintainers,

We've started work on SDL-0234. To prepare the technical specification, here are some questions for you:

1. The Python defined as a strict requirement in the proposal for the generator script, but the version is not set. Because Python 2 support ends in 2020, in our opinion, version 3 is preferable. Could you approve that?

2. Based on the review of the current RPC classes in the library, we have found the code for enums, structs, and messages (request, response, and notification). Meantime response only has the constructor and no additional methods, is that enough to prepare "XML -> Source Code" transformation rules? Do you have additional rules which are not reflected yet in the existing code?

3. Core RPC classes (RPCStruct, RPCRequest, etc.), What code was used as a base for them and for the RPC classes structure, do you have the specification to be sure generated RPC classes extend core classes correctly and placed in right folders?

4. The generated RPC classes will be brand new in this library and don't have existing test coverage yet, unlike other libraries. It seems unit tests also can't guarantee the generated code is working and clear that RPC classes need functional tests for this purpose. Moreover, based on the current state of the library's code, it's impossible to check generated RPC classes in communication with SDL Core because required managers are not completed. Based on the facts above, what are the definition of done and acceptance criteria for the generator and its result? Do you have any ETA for the minimal valuable product state of the library?

5. To define the general command-line switches, we propose the following list. Please approve or extend this list with additional general or library-specific switches:

• <input-file> - required, should point to MOBILE_API.xml
• <output-directory> - required, define the place where the generated output should be placed
• [<regex-pattern>] - optional, only elements matched with defined regex pattern will be parsed and generated, if present
• --help, -h - print the help information and exit
• --version, -v - print the version and exit
• --verbose - display additional details like logs etc.
• --enums, -e, --structs, -s, --functions, -f - only specified elements will be generated, if present
• --overwrite, -y - force overwriting of existing files in the output directory, ignore confirmation message
• --skip, -n - skip overwriting of existing files in the output directory, ignore confirmation message

[theresalech]

@vladmu Regarding questions 1 and 5, these will have to be included in the Evolution Proposal and voted upon by the Steering Committee. We would suggest leaving these comments on the revisions proposal currently under review: smartdevicelink/sdl_evolution#853.

We will respond to questions 2, 3, and 4 soon.

[TinaKleczka]

@theresalech Any ETA responses to questions 2,3, and 4? Thank you

[joeygrover]

2. If you look at the response that was created in the project and compare it to the RPC spec you can clearly see it has no additional params beyond that of every response. Therefore, there is nothing but the constructor. If a response has additional params outside the ones contained for every response obviously they need to have getters and setters just like they do in the request and notification classes.
3. Follow what exists in the project. Messages, structs, enums all have their own folder where messages are all requests, responses, and notifications. The core classes do not need to be nor should be generated. They should be left alone unless there is an actual need for modifications that should be clarified first.
4. Unit tests should be created to test the RPCs. This should actually be enough to test the classes as they are glorified JSON objects in JavaScript. We are working towards a working library as we speak and you can follow the progress in the project tab.

[vladmu]

2 . Thank you for the clarification, we already caught this logic before your answer, but till we read the source code of the RPCResponse class, it was not clear that "success", "resultCode" and "info" properties always present in all responses. That's why the next question (point 3) was asked about the source and the specification of core classes.

3 . The question is not in the generation or required changes of core classes but the correct extending them by generated classes (e.g., a case from point 2). Thus the specification or understanding which classes from what library are references for the current realization of the core classes could immensely help us in the creation of a generator.

We understand that the work with those core classes is still in progress, so the question is still open, and if you could point us at least to references of the base classes, it would be useful, meantime the specification is better of course.

If the core classes are fully done, just confirm that, and we will base our work on their source code without additional questions.

4 . From your answer, we understand that the test coverage of the classes, produced by the generator, is the one point of DoD, and without that coverage by unit tests, the RPC generator task could not be accepted as done? Please confirm that.

It sounds like we won't test functionality starts from preparing and sending RPC requests and receiving RPC responses. Do we correctly understand that if we could make sure by unit tests the produced RPC class prepares the correct internal JSON structure by getters and setters, it would be enough?

And the follow-up question regarding the test framework we should use for that test coverage. Seems it would be correct if the whole library follows the one tool, but currently we don't see any existing coverage or tools selected for unit tests in the repository. Do you have a selected test framework or we could propose? What is the correct way to put the test framework into the library, I mean the workflow? Should it be a separate sdl_evolution proposal first?

[TinaKleczka]

@theresalech Please review above comments and provide any additional information. Thanks.

[joeygrover]

Please try to understand we are answering these questions as soon as we can. If Ford has any insight to help with their contribution that would be great as well!

2. 👍
3. You can use what is in developas reference. If we need to make changes to the output of the generator when we find an issue we will let you know or request the change.
4. I don't believe unit tests or tools are in scope of the proposal. We are open to suggestions. Pending when this is submitted it can be possible to test with what is in the develop branch.

[vladmu]

@joeygrover thank you for answers, only point 4 is not fully clear, did you mean the scope of the generator proposal or library itself? Should or should not unit tests of the generated code be included as "Definition of Done" for the RPC generator script work. This is important because the volume of work is different depends on the answer. And we want to be sure the maintainer's expectation is fully covered before provide you the PR with the generator code. As I said we didn't find any testing tools or coverage in the develop branch nor any other branches currently, so we can only compare the generator result with existing classes, I'm not sure if it is enough.

[joeygrover]

The proposal itself does not contain any language about creating unit tests or a testing platform. It should be a question added to the currently in review revisions. The SDLC should decide if they think the output should also include the unit test.

[ghost]

I think that unit tests are necessary for ensuring the code quality of the scripts. These unit tests could test the scripts output to ensure the code produced is as expected. I don't think that unit test code should be produced with the actual RPC classes unless we extend the RPC classes but as suggested we can talk in the next SDLC meeting.

[vladmu]

@joeygrover we have found the usage of the new method setValue for RPCStruct children, which does not exist in the parent class nor the classes where that method is used. I'd like to ask some questions regarding this and some others:

1. Back to my previous question, do you have additional XML -> Source Code transformation rules which are not reflected yet in the existing code? If we will base the generator only on existing code in develop, it is hard to follow up with each unexpected functionality like the new setValue method. We'd like to avoid such situations in the future.
2. This setValue doesn't look as edge cases. Do we need to follow the new setValue method in the generator? How and in what cases? As this method does not exist, we can't grab the logic from the existing code and get the correct answer without your help.
3. We are working on the specification of XML -> Source Code transformation rules in parallel with the generator coding. Before we could send it to your review, it would be helpful to get at least the correct cases for the usage of setValue, setParameter, getParameter, getObject and validateType methods. Because the reverse-engineering of the transformation rules from the existing code seems don't effective.
4. I should ask again the document for code standards and the naming convention the JS library should follow. Numerous questions in PR Add base RPCs and supporting classes #24 were exactly regarding this, like the private property name prefixed by _ (eg. _MAP), SCREAMING_SNAKE_CASE names of enum elements and related methods, etc. Also, we need some instructions in what cases we should transform the values from the XML based on that conventions document and in what cases the value should stay as is (eg. question about internal_name).
5. The conversation in PR Add base RPCs and supporting classes #24 was closed as offtopic, and I suppose it was due to numerous questions we asked and due to the last problem regarding setValue in fact Add base RPCs and supporting classes #24 (comment). By the way, we still have open questions. We should have a clear understanding of the new provided code is correct before putting new transformation rules based on it into the generator. And, I suppose, if the code is really wrong, our remarks are not redundant for your work. Classes added in this PR have a direct relation to our part of work with the generator, and we need to have a possibility to provide our remarks and ask questions in time, and in lines of code where mismatches are found, it is crucial. I'd like to ask, do not block our questions and the possibility to ask them, as this is required for both sides, you and us, to have the best result at the end.

CC: @kshala-ford @TinaKleczka @joeljfischer @nickschwab

[joeygrover]

• setValue doesn't exist it's a typo; use setParameter. I will remove occurrences.
• Private method members should have an underscore in front of them, this includes _MAP, see the contributing guidelines in the repo.
• We have a task to hopefully clear up some of the items of the RPC spec's poor continuity. But the altering the generator script should be easy and intuitive. Changing something like setValue to setParameter is a couple second change. Or when accessing an enum value changing the output method name from enum.name to enum.internal_name.
• The frequency in which RPCs are going to be "overwritten" should be very small. I don't think it is on the generator to cover all corner cases because of this.
• The PR was closed because it was a PR to include some basic RPCs that could aid in testing. I had previously stated that the RPCs included in that PR would be overwritten. The questions on RPC generation should happen here for all to track.

[vladmu]

@joeygrover thank you for the quick reply, the summarizing based on this:

1,2,3 - We will continue to follow the existing code in the develop branch. We have provided the correct remarks in PR #24, the setValue method is a typo, and it was merged into develop by mistake.
4. Thank you for pointing to guidelines, we were looking exactly for this. Here is a link: https://github.com/smartdevicelink/sdl_javascript_suite/blob/develop/GUIDELINES.md
5. We should avoid discussing the RPC generator questions in other places than this issue. That's the correct point, and we will keep this in mind every time we write a comment. Thank you.

[theresalech]

Proposal markdown file has been updated per the revisions included in accepted Evolution Proposal SDL 0234 Revisions - Proxy Library RPC Generation. Please reference issue and proposal markdown file for more details.

[vladmu]

@crokita @nickschwab based on this comment the branch of lib/rpc_spec submodule should point to develop branch instead of master. I'm not sure about the correct workflow for this change here. Could you make this in a proper way?

[joeygrover]

@crokita @nickschwab @vladmu I would recommend two PRs. The first PR should point to the develop branch of the RPC spec so that we can continue development accurately. The second PR should point to the master branch and before we release this library we should merge that after we merge the RPC spec develop branch into master.

[vladmu]

@joeygrover Are there required additional issues to be created here before creating PRs? Could I do it by myself?

[joeygrover]

@vladmu for this specific item I don't think we have to create issues. Once the PRs are created I can add them to the 1.0 project on GitHub.

[crokita]

@vladmu
The JS library will need to align with the other libraries in avoiding reserved keywords that they also use when generating RPC definitions. Therefore, RPC Generators in all platforms (iOS, Java Suite, JavaScript Suite) should avoid using keywords which are now in the RPC Spec repo 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 # or ## or ### …). Note that some reserved keywords have a # without a 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.

There are additional requirements specific to the JS suite library, noted below:

• Create a general rule in JavaScript to uppercase the first letter of any types (applies to messageType so that it is now MessageType)
• Remove numeric values for enums and only generate string values for them. Ex:

PredefinedWindows._MAP = Object.freeze({
    'DEFAULT_WINDOW': 'DEFAULT_WINDOW',
    'PRIMARY_WIDGET': 'PRIMARY_WIDGET',
});

[theresalech]

Proposal markdown file has been updated per the revisions included in accepted Evolution Proposal SDL 0234 Revisions - Proxy Library RPC Generation. The revisions include adding a rule that takes into account a set of keywords that currently exist in any of the three client side libraries (iOS, Java Suite, and JavaScript suite), and avoiding creating method signatures that collide. Please reference the issue and proposal markdown file for more details.