Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add app-proto service with submission proto, helpers #493

Merged
merged 29 commits into from
Oct 22, 2021
Merged

Conversation

haworku
Copy link
Contributor

@haworku haworku commented Sep 24, 2021

Summary

Adds app-proto service and a submission.proto definition. Also includes utilities to encode/decode from Typescript domain model.

Related issues

https://qmacbis.atlassian.net/browse/OY2-12158

.proto file general notes

  • not using required at all
  • repeated means this is an array
  • field numbers 1-15 are for the most accessed fields (see docs on field numbers)

.proto file styles and formatting

See styling guide - we are linting on this with protolint. It is in the precommit config, also can be added as vscode extension.

  • repeated fields are pluralized (have an s)
  • enums use prefix specific to their field name. Use UNSPECIFIED as default value. Field names are CamelCase (with an initial capital)and values are CAPITALS_WITH_UNDERSCORES. see docs
  • order at top of file matters.

To Do:

  • add support for the date mm/dd/yyyy format we use for certain dates
  • consider if pulling out separate proto files is helpful - if so, on what boundaries
  • figure out helpers and tests

@haworku haworku requested a review from macrael September 24, 2021 14:52
Copy link
Contributor

@macrael macrael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job laying out the submission in proto form, I put some questions inline but it looks like it’s all there, to me.

Generation:

  • to match app-graphql, I think that there should be a command that installs the generated files inside of app-api/gen/
  • Also, we should have a watching version of that command, I forget what that npm tool you used to do that with the proto/avero exploration project but that would work here. That way we can call that from ./dev like we do with the graphql compiler
  • I think the conversion methods (+ tests) between domain model and proto could live here or could live in app-api which is where we’re going to call them.

Coding

  • I don’t really like “helpers” as a name here, the functions are going to pretty specifically be involved in encoding / decoding so some kind of name that matches that would be better

Testing

  • There’s a couple different ways we could do some tests. The easiest way to start is to get some sample valid domain models (do we have some of those for the api tests already?) and write tests that just take that, serialize it, deserialize that, and confirm that what came out is the same as what went in.
  • After we’ve got the basics working, we could write some tests that confirm that we can read serailized versions of the data. We can generate a bunch of serialized DraftSubmissions and save them as test files, then we can write tests where we deserialize them and check that we get what we expect out.

services/app-proto/src/proto/submission.proto Outdated Show resolved Hide resolved
services/app-proto/src/proto/submission.proto Outdated Show resolved Hide resolved
services/app-proto/src/proto/submission.proto Outdated Show resolved Hide resolved
services/app-proto/src/proto/submission.proto Outdated Show resolved Hide resolved
-  enum use C++ scoping rules - enum values are siblings, not children
… closely, add some missed fields to .proto file
@macrael
Copy link
Contributor

macrael commented Sep 30, 2021

Re: optional stuff. I also dont’ totally understand what the tradeoffs are. It seems like from reading that article that without “optional” it’s not possible to query the proto message to see if a field even exists in that message as compared to it just being unset. I’m not sure how that plays out in our usecase though, so if it’s not getting in your way I’d say ignore it and move on. We can add it back later if we decide we need it.

@macrael
Copy link
Contributor

macrael commented Oct 18, 2021

I believe this is ready for review. Changes since @haworku put it down:

  • I moved all the code into /common-code, I think that keeping the package there with tests inside makes the most sense for its future use
  • I picked a strategy for dealing with the types of our different things. Since every single field in the proto can be null or undefined, when we are constructing our domain models where that is not the case, we instead construct at RecursivePartial. That way we get types at least for all the fields, and then after we have that we validate that everything that is required is present
  • Fundamentally, we are converting between two different types at runtime. That sounds like we need a runtime validator, like yup.
  • That validation is being done with Zod for now. Zod is very much like yup, so I'd like for us to standardize on one rather than the other. For now I'm trialing zod b/c it's more typescript aware, just to compare them. The likely move is to just use yup everywhere since we're already relying on it in our forms. Might lose a little type safety but that's probably alright.

@macrael macrael marked this pull request as ready for review October 18, 2021 23:33
protoDateTime: statesubmission.IStateSubmissionInfo['createdAt']
): Date => {
return protoDateTime
protoDateTime: statesubmission.IStateSubmissionInfo['updatedAt']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we export and unit test these types of functions? feels like there significant branching logic here

console.log('RATINGE', JSON.stringify(rateInfo))

// pull out the actuary contacts
if (rateInfo.actuaryContacts) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we pull out something like calculateRateInfo as a function instead of everything happening inline. There's a lot going on here. That way cleanedRateInfo = calculateRateInfo(rateInfos)

@haworku haworku changed the title [wip] Add app-proto service with submission proto, helpers Add app-proto service with submission proto, helpers Oct 19, 2021
Copy link
Contributor Author

@haworku haworku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good. Tried to add more descriptive test assertions - feels free to change or ignore. I cannot approve this since it is my branch so @macrael will have to approve and merge.


These functions encode and decode our domain submission types to and from protobufs. Our proto schema is located in the [app-proto service](https://github.com/CMSgov/managed-care-review/tree/main/services/app-proto) where it is compiled as part of our build process into the /gen package as a pair of js and d.ts files that allow us to read and write conforming protobuf bytes.

The work of this package then is in safely converting our domain DraftSubmissionType and StateSubmissionType to and from the generated StateSubmissionInfo class that can be encoded and decoded to protobuf. This is not a simple mapping for a few reasons.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this listing of the reasons why transform functions are needed. That especially feels like something we could quickly forget

Copy link
Contributor

@macrael macrael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this per @haworku

@macrael macrael merged commit 4a21685 into main Oct 22, 2021
@macrael macrael deleted the hw-protobuf branch October 22, 2021 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants