Skip to content

Conversation

gyDBD
Copy link
Contributor

@gyDBD gyDBD commented Jul 1, 2021

Feature

Add a new config propertyBatchKey so that we are able to batch “properties” call.
Only a specific kind of endpoint would benefit from this:
input parameters format:

ids: str[] (should be the batchKey)
properties: str[] (should be the propertyBatchKey)

response:

properties is not returned in a nested object, but spread at top level. (see getFilmsV2 in example/swapi)
e.g.: { id: 'id_1', property_1: 'foo', property_2: 'boo' }

Status Quo

run node build/swapi-server.js under example/swapi, the getFilmsV2 function will be called three times in a roll.

after this change

run node build/swapi-server.js under example/swapi, the getFilmsV2 function will be only be called once.
And the flow type of response
Screen Shot 2021-07-01 at 10 40 59 AM

Manual testing enforce optional properties

https://fluffy.yelpcorp.com/i/18BHFxFfJg0TgsWjWDwngS2c6D4WS8zS.html#L1,L7,L12,L15,L29,L35,L57

Note

I realized it's hard to build something perfect that works for different kinds of use cases. And it adds complexity for both reviewers and me. Given we don't have much time left for this project, I think we should only focus on to support the use case of batching SBP services calls and maybe in the future we can improve this more.

(Previous pr: #261)

@gyDBD gyDBD marked this pull request as ready for review July 1, 2021 21:27
@gyDBD gyDBD requested review from kkellyy and magicmark July 1, 2021 21:27
@gyDBD
Copy link
Contributor Author

gyDBD commented Jul 21, 2021

Only reponseKey (they key in the response that correspond to batchKey, usually id) can be marked as required in the swagger definition. The implementation is undersrc/index.ts and manually tested https://fluffy.yelpcorp.com/i/18BHFxFfJg0TgsWjWDwngS2c6D4WS8zS.html#L1,L7,L12,L15,L29,L35,L57

@gyDBD gyDBD requested a review from magicmark July 22, 2021 17:02
src/index.ts Outdated
Comment on lines 53 to 56
/**
* Throw erros when resource uses propertyBatchKey but doesn't match requirements
*/
function verifyBatchPropertyResource(args: CLIArgs) {
Copy link
Collaborator

@magicmark magicmark Jul 23, 2021

Choose a reason for hiding this comment

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

As discussed, since this is quite Yelpy and relies on checking swagger stuff specifically (which this tool is currently opinionated about), I'd suggest keeping this logic but moving it to a seperate script that runs in our build, outside of this tool.

added benefit: we can remove the swaggerPath / httpMethod keys etc since we'll know how to map to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, you mean move the script to ggs? But the propertyBatchKey needs to be in dataloader-codegen, if anyone else uses propertyBatchKey they won't have correct flow type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nvm, saw your other command!

@gyDBD
Copy link
Contributor Author

gyDBD commented Jul 26, 2021

script to enforce the swagger contract will be moved to Yelp-land. The review comments on those lines will be addressed there.

@gyDBD gyDBD requested a review from magicmark July 26, 2021 23:14
Copy link
Collaborator

@magicmark magicmark left a comment

Choose a reason for hiding this comment

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

fix n ship, awesome stuff! 🎉🎉🎉

Copy link
Collaborator

@magicmark magicmark left a comment

Choose a reason for hiding this comment

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

fix n ship, awesome stuff! 🎉🎉🎉

@gyDBD
Copy link
Contributor Author

gyDBD commented Jul 28, 2021

@magicmark Hey Mark, thanks so much for the review!! I've addressed all your comments,do you want to take a final look?

@gyDBD gyDBD requested a review from magicmark July 28, 2021 23:56
Copy link
Collaborator

@magicmark magicmark left a comment

Choose a reason for hiding this comment

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

🥳 🎉

@gyDBD gyDBD merged commit c6a05d7 into master Jul 29, 2021
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