refactor: remove resolvers, remove promise in withBeginTransaction, simplify code calling withBeginTransaction #1212
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change the resolvers so that they all follow the same pattern.
Generalize the calls to withBeginTransaction. Don’t allow the error type to be undefined in UserCallbackData since the error never actually will be undefined. This is necessary to solve various compile time errors.
commit and runQuery can now use #wrapWithBeginTransaction and this will reduce the amount of code necessary in each function respectively.
This function replaces a common pattern that occurs in transactions where we provide a callback that accepts an error as the first argument and some generic type that extends an array of any as the rest of the arguments.
Adding a return type just makes type checking stronger in the client library. We should introduce it here to make it more likely a compiler error will be thrown if the function is misused.
When playing around with code to return a standard callback, code was written that is not required anymore. Also, rename StandardCallbackArgumentsAny.
The UserCallbackArguments do not need a separate type if they are only used once. Define the type inline where it is needed.
Error or null is only used in two places so inline it here.
callbackWithError is an abstract function that needs to be explained. This comment helps with that.
sendUserCallbackData needs to be described so that its purpose is clear.
Move function to the right order alphabetically
Do not define a separate variable for only one use.
Replace with Function to eliminate references to the generic parameter which make CI tests fail.
No need to mention the argument types and return types.
withBeginTransaction is only called by one function so remove it to condense call stack.
Eliminate the need to build a resolver for all code usages of withBeginTransaction. Eliminate the need for a promise in withBeginTransaction. Eliminate the need for a callbackWithError function.
It is not used.
Rename the function and eliminate redundant code.
Update the comments to reflect new parameters.
product-auto-label
bot
added
size: xl
danieljbruce
changed the base branch from
main
to
nodejs-transaction-redesign-feature-branch-from-main-github-kokoro-package-json
danieljbruce
changed the base branch from
nodejs-transaction-redesign-feature-branch-from-main-github-kokoro-package-json
to
nodejs-transaction-redesign-feature-branch-decorators
danieljbruce
changed the base branch from
nodejs-transaction-redesign-feature-branch-decorators
to
nodejs-transaction-redesign-feature-branch-decorator-add-decorator
danieljbruce
changed the base branch from
nodejs-transaction-redesign-feature-branch-decorator-add-decorator
to
nodejs-transaction-redesign-feature-branch-5-adding-the-tests
product-auto-label
bot
added
size: m
Move withBeginTransaction to last alphabetically
| } | ||
| }); | ||
| } catch (err: any) { | ||
| return callback(err); |
There was a problem hiding this comment.
What kind of errors might you encounter here? A comment would be useful
src/transaction.ts
Outdated
| try { | ||
| await this.#mutex.runExclusive(async () => { | ||
| if (this.#state === TransactionState.NOT_STARTED) { | ||
| const runResults = await this.#runAsync({gaxOptions}); |
There was a problem hiding this comment.
nit: it looks like runAsync's primary purpose is to call beginTransaction. Maybe name it something like beginTransactionAsync?
There was a problem hiding this comment.
Sure, changed to beginTransactionAsync. Since this is in the transaction class run typically means run the transaction, but maybe beginTransactionAsync is more descriptive. Either way, this method is private so the name can be modified later if need be.
daniel-sanche
approved these changes
Dec 13, 2023
In the code block it is not clear what kind of error will be produced and caught in catch so a comment is added to clarify the type of error that gets produced.
Calling the method `run` is too ambiguous. Use #beginTransactionAsync instead.
danieljbruce
merged commit 487d8de
into
nodejs-transaction-redesign-feature-branch-5-adding-the-tests
13 checks passed
danieljbruce
deleted the
nodejs-transaction-redesign-feature-branch-remove-resolver
branch
danieljbruce
added a commit
that referenced
this pull request
Feb 12, 2024
* Add a unit test for commit as a non-tx The fact that using a transaction object to do a commit results in a non-transaction should be documented so that if we decide to introduce a change later where this behaves differently then it is well documented. # Conflicts: # test/transaction.ts * use linter * Write tests to capture current behavior of error When begin transaction sends back an error, we want some tests to capture what the behavior is so that when we make changes to the run function then behavior is preserved. * Add tests for passing a response A response should reach the user the right way. Add tests to make sure behavior is preserved. * run async close to working In the run function delegate calls to runAsync and use run async to make promise calls * Add runAsync to promise excludes This allows this function to return a promise instead of a promise wrapped in a promise. This makes the tests pass and behave the way they should. * Remove space * Change to use this instead of self Do not call request from self * Eliminate unused comments * Add two comments Comments should actually explain what is being done * Remove the commit test for this PR The commit test for this PR should be removed because it is not really relevant for the async run functionality. * Clarify types throughout the function The types used should be very specific so that reading the code isn’t confusing. * Add a bit more typing for clarity Add a type to the resolve function just to introduce more clarity * Change types used in the data client callback Make the types more specific in the data client callback so that it is easier to track down the signature and match against the begin transaction function. * run the linter * Add comments to clarify PR * Refactor the parsing logic out of run The parsing logic is going to be needed elsewhere so taking it apart now. * Change interface of request promise callback The interface name should be changed so that it matches what it is. It is the callback used to form a promise. * Move commit functionality to a new function The internals of commit should be moved to a new function. This way we can sandwich a commit async call between commit and runCommitAsync. * Add the mutex and transaction state etc. Still an issue with system tests. commit still needs tweaks to work with async. * Remove no-op, get commit tests working Remove the no-op, get commit tests in place. * Add mocks and additional debugging Mocks and additional debugging hooks to introspect what is going on. * Add tests for commit Make sure commit behaves the same way as before. * Add the commit tests Commit tests added to ensure that commit behaves the same way as before. * Fix the tests so that they pass on commit The tests should pass before we make changes to commit. * refactor one of the mocks One of the mocks does not need to be written twice * reverting changes to add new test on transaction * Change the promise Make the promise simpler. Change the tests to exclude functions with promisify. * Hide data completely Change accessors to hide data completely instead of using the private modifier * PR use if/else block Eliminate the early return as suggested in the PR * Add comments to document the new functions The comments capture the parameters and return type. * Update return type in docs * Update the tests to include runAsync runAsync should be in promisfy excludes * refactor: Break transaction.run into smaller pieces for use with async functions and other read/write calls * Rename a function to be more descriptive Make sure it is explicit that we are parsing begin results. * Move the mutex and the state down to derived class The mutex and state should be moved down to the derived class. We are going to override get/runQuery/runAggregateQuery there. * chore(deps): update dependency @types/sinon to v17 (#1197) * Add hook to call run before commit Add the hook to call run before calling commit in existing tests. * Add commitAsync to promisify excludes commitAsync should be resolved and then() function should be called as it was not being called before * remove the console logs * Delete run commit * Remove the private identifier Use the private modifier instead to hide data * Add withBeginTransaction withBeginTransaction will be used with all calls that begin transactions and then intend to use the mutex for locking when the begin transaction call is made. * Add another level of abstraction Add #beginWithCallback so that all the read calls can be made with one line of code. * commit async is not needed anymore * This data structure is not needed anymore * Replace types associated with run to use generics The generic parameter should be used for types with run. * Make response type more specific for parseRunAsync More specific types are better and it make code easier to read * chore(deps): update dependency @types/is to v0.0.25 (#1198) [](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@types/is](https://togithub.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/is) ([source](https://togithub.com/DefinitelyTyped/DefinitelyTyped)) | [`0.0.24` -> `0.0.25`](https://renovatebot.com/diffs/npm/@types%2fis/0.0.24/0.0.25) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: Branch creation - "after 9am and before 3pm" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/googleapis/nodejs-datastore). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40Ni4wIiwidXBkYXRlZEluVmVyIjoiMzcuNDYuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> * Modify comment Modify comment so that it doesn’t reference the way the code was before. * Add implementation for runQuery runQuery should make the call to begin the transaction first if that hasn’t already happened yet. * Making fixes for runAggregationQuery Fixes for run aggregation query. Still getting undefined results. * Write tests for runAggregateQuery Make sure that runAggregateQuery return results make it back to the user. * Get one test case passing for runAggregateQuery runAggregateQuery should not be excluded by promisify. Otherwise it will not return a promise, but we want it to return a promise. * remove console log clutter * Change tests for runAggregationQuery Change the test to use deep strict equal since the objects being compared will not be reference equal. * Add resolver type Eliminate some unused code * Create a transaction wrapper class for testing This is going to be a useful test for mocking out various layers to make sure they work the same way as before. * Clean up mocked transaction wrapper The mocked transaction wrapper should reset all mocked gapic functions and not have to be told which ones to reset. * Move test information for commit into commit block * Add gapic mocked tests for aggregation query Gapic mocked tests for run aggregation query need to be written that use the mock transaction object. * Fixing up the runQuery test * Finished the runQuery tests The runQuery tests are finished so we can use this to take apart the function from end to end. * Finished the get tests The get tests now make sure that the data coming back from the gapic layer results in the same values for users. * remove only * remove only and console log * Try modifications to runQuery Add modifications to runQuery to use some function to return values * Add try catch blocks to handle errors in the tests try/catch logic in the test suite is added so that errors bubble up to the test runner and it is easier to see why tests failed * Modify commit so it doesn’t run early Don’t start commit with a promise or the promise will run early. * Update get with resolver get should use the same pattern as runQuery and runAggregationQuery to use a resolver for the mutex business logic * remove #beginWithCallback #beginWithCallback is no longer used so remove it * Remove #withBeginTransaction This function is no longer used * Rename #someFunction with begin transaction #someFunction should be named differently * Fix promisify. Change to a deepStrictEqual check. A deepStrictEqual check is all that is needed in this test that currently reuses a transaction. * Add setImmediate to the tests If we add a delay to the tests then the mutex has the opportunity to unlock before running the tests. * Added some tests for call ordering We want to make sure that the calls get delivered and received in order * Pack testing tool into an object Put the testing tool in an object so that we can explore more orderings and possibilities. * Eliminate console logs, use expected order Allow expected order to be passed into the tester. This will make the object reusable. * Add a check for transaction not started yet Need a check to be sure that the transaction is not in progress if making another beginTransaction call. * Remove NOT_TRANSACTION Remove the NOT_TRANSACTION and default to NOT_STARTED * Remove only Remove only and let all unit tests run * Use an enum instead of static class members Use an enum to track transaction state. * TODOs are done * Move excludes to proper position Excludes should contain the right functions in the right order. * Simplify the run function a little bit Regroup functionality to simplify the run function * Add comments to tests Explain purpose of testing objects * Modify tests and fix a bug from the merge The merge added a bug because the pound sign wasn’t used and it should have been. * Fix error message * Comments and general cleanup Remove constructor parameter that is not used. Add comments to functions so that the tests are more readable. * Added comments for functions in transaction.ts Functions in transaction.ts need JSdoc comments. * Add a comment for aggregate queries Add JSdoc comment * Add an assertion to test The assertion should make sure the first read returns undefined. * Add tests for lookup, put, commit For calls with run and without run, do lookup, put, commit. Make sure to clean up after the tests too. * refactor the test Test does lookup, put, commit. This is done in an async function. * Add tests for put, lookup, commit A group of tests should be added for put, lookup, commit. One with run and one without run. * chore: update cloud-rad version to ^0.4.0 (#1199) Source-Link: googleapis/synthtool@1063ef3 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:e92044720ab3cb6984a70b0c6001081204375959ba3599ef6c42dd99a7783a67 Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * Add comments and general cleanup Cleanup includes adding comments so that the tests are more readable. * Remove some redundant tests Some of these tests are captured in the various functions describe block. * Correct the comment to be more accurate The comment should talk about what the tests actually intend to do * General improvements to code quality Move declared transactionWrapper and other variables out to shorten code. Remove require and replace with import. Use more specific type. * Add data client check Add data client check and add type for data client. * Eliminate the mocked function variable The mocked function variable is not used so remove it from code. * Move begin transaction setup code The same block of code for setting up begin transaction is repeated twice. Move it into one function and use it from both before blocks. * Replace the TODO for the key Add some try blocks to make the errors more visible also. * Update description Update the description for the test to give a better explanation of what the describe block does. * Add comments to describe the purpose of signaller The comments describing the callback signaller should explain the problem it solves. * Add both dones back in The done functions were lost in the refactors. Let us add them back in. * mockedBeginTransaction should be Function Make a more specific type for mockedBeginTransaction and the functions mocked. * Use ECMA script modifier Make sure this function is completely hidden. * Remove TODOs that no longer apply The TODO statements no longer need to be followed up on so remove them. * beginTransaction type definition Remove the type definition. It is not used. * Add the setupBeginTransaction method Refactor test code to provide better error catching and also not repeat setup code for the run function. * Add a comment for private runCommit method The comment for the runCommit method just captures the fact that the function is used as a pass-through. * Modify comment to be more clear * Update comments The comments should more directly address the problem the tests are meant to solve * Eliminate redundant test code This describe block is now duplicated. Remove it. * revert comment This was a typo * refactor: Move commit logic and add tests that prepare for transaction function changes * runQuery, put, commit Another some more integration tests for runQuery, put, commit * run linter and group some tests into a describe block so that they don’t run before some new tests we are going to add. * Add some runAggregationQuery integration tests Add one of the test cases from the document (runAggregationQuery tests) * Add tests for put, runAggregationQuery, commit put, runAggregationQuery, commit tests have been added and now run properly. * Write some latency tests The latency tests measure time taken with and without using the run call. * Add logs to make latency tests run The logs will output the time required for the latency tests. * Add two tests for put, commit The two tests for put, commit should make sure that begin transaction is called. * Build requests into the transaction order tester To meet the needs we want for unit testing we must add some checker that lets us record the requests and verify that they are the right values. * Modify order testing Modify the order tester object to include more calls. Also add expected requests to the existing tests. * Create lookup, lookup, put, commit Create test for a read, read and then commit. * Fix the unit test Unit test should capture the fact that the run callback is used. * Add lookup requests When using get requests without passing consistency, we should see two lookup requests reach the gapic layer. Consistency should be removed from the test because it is not meant to reach the Gapic layer. * Remove the console logs console logs are not needed. Remove them so that the tests are cleaner. * Remove only * remove the latency tests The latency tests are not needed anymore so remove them. * Separate into a section transactions with/without A section name transactions with and without run should be used. * Remove a describe block and run linter This should make the diff a lot cleaner * Re-introduce a test that was there from before We don’t want the diff to say that we have removed tests. Therefore, add the test that was there before. * Update comment Modify the comment so that it is more objective * Add a comment for the runAggregationQuery function The comment for the aggregation query function in transaction.ts is needed here. * Simplify argument type In #withBeginTransaction there is a data type that matches the complex data type in the second argument of this function. * remove unnecessary extra variable The extra variable is not needed * Eliminate redundant definition Eliminate a definition and use the commit response data type here instead. * Add a comment for the #withBeginTransaction fn Need to clearly explain what the withBeginTransactionFunction does * Eliminate a line that was used for debugging * Add space back in * remove indent * Add empty line back in This simplifies the diff a bit * Remove ambiguous pronoun * Shorten comment * Add a comment for begin Add a comment that incorporates how this function interacts with #withBeginTransaction. * Shorten comments Don’t repeat information from the comments in the super class. Just mention the super class comments. * Type is only used once Inline the type to eliminate redundancy in the code. * Inline another promise type Inline another promise type which lets us eliminate one line of code. * Rename type to executor Executor is what it is called in the promise argument. * run linter * Simplify the data types more The types should line up for the executor in the runAsync function * Rename type to describe how it is used This name is clearer * Rename to userCallbackData. Add comments UserCallbackData is a more specific description of the type of data the generic type is. We also need comments to explain what these interfaces should represent. * resolver is a better term to use here * Change the type in the comments The type has updated since to be a more general type. * Flip if/else Flip the if else. Improvements are that the if checks for the affirmative and the longer block comes first * run linter * Add brackets Introduce brackets so that the word matching the parameter is more isolated. * Stronger type checking for runAsync Use more specific types in the runAsync function so that the compiler will complain if anything isn’t right. * Call the function fed into the promise resolver resolver should be the name used just to be consistent with all other code. * Rename variables and add comments Rename the variables so that the relationships between them are easier to understand. Add comments explaining the purpose of the variables. * Add comment for the done function * Make checkForCompletion private To avoid confusion make checkForCompletion private so that the user of the object does not need to think about using it. * Replace all events with enum values The events should not be strings. If they are strings then we might be magic string matching and this should not be done in code. * Make code more succinct Remove the need for a map. Remove occurrences of strings throughout tests and replace them with enum values. * Rename event to UserCodeEvent Call this enum UserCodeEvent as it is a better description of what the values could potentially be. * Remove the map from string names to gapic layer This map is not needed anymore because now we use the Gapic layer function event directly. * More specific type Shouldn’t just be a string. This should be a Gapic Layer function. * todo is done Assertion check is done * Eliminate need to define extra variable This just makes the code a bit more concise. * Add comments to describe test objects The comments describe Gapic layer data passed back. * There is no need to make these functions private Just inline the callbacks. A private function makes their relationship with the code that uses them confusing and each one of them is only used in one place. * Renames all callbacks to just callback Rename all callbacks to callback because that is the variable name inside the client function. * Take requests private * Take expectedRequests private expectedRequests is not used outside of the transaction order tester so we should take it private. * Take expected event order private Expected event order should be taken private. * Take event order private event order is not used outside this object * Fix some warning messages Various warnings show up in the es-lint. Fix them. * Use unknown for Gapic Layer Response Change the type to unknown to address the ES-lint error. * Get rid of any type for runQuery Use the more specific type provided by runQuery instead. * More linter fixes Addressing some of the problems in the new tests according to ES-lint. * Change parameters to match the get callback Stronger type enforcement helping us catch errors later. * Another ESLInt correction Use GetCallback here instead of any for the Get response type. * Remove query variable as it is not used * Take done private When done is passed in and saved it should only be used internally. * Eliminate unused arguments in callback Eliminate the arguments because if they are not used then they cause ES-lint errors. * Add readOnly reserved word Various properties can be readOnly as recommended by the linter. * Make type for done explicit Explicitly call out the fact that done should equal mocha’s done function. * Another place where done should be mentioned The done type should explicitly be mentioned here. * No need to access order tester to get wrapper There is no need to access the transaction wrapper to get the order tester. Just access the transaction wrapper directly. * private variable transaction wrapper Take transaction wrapper private. It does not need to be visible to the user. * Capitalize RequestType Change RequestType to be capitalized and use it whenever a request needs to be made. * request should be a more specific type RequestType should be used here. * Do info checks for error and response cases Add another assertion check. Do the check for the error and response cases. * These are actually results from runQuery * Eliminate extra line of code Add assertion check for info. Eliminate the code that stores all the results. * rename enum The word Layer is redundant. Also emphasize that the enum captures the function name. * Lookup response The variable should include the word lookup since technically that is the name of the Gapic layer endpoint. * Add comments for the transaction order tester Explain each function with a comment describing what it does. * Introduce a new push function Add a push function so that in each test it is easier to see the events line up. * Remove middle layer functions Remove functions in the transactionOrderTester and use functionality from those functions directly because then in the test it is easier to read the test’s intent. Also, add GapicRequestData type to simplify types in the test class. Shorten transactionOrderTester to just tester. * Prefer block comments The comments are multi-line so use block comments instead. * Rename event to CUSTOM_EVENT CUSTOM_EVENT should be used instead of FUNCTIONS_CALLED. FUNCTIONS_CALLED is sort of ambiguous. * Add square brackets to comments square brackets added to make the comment more readable and user friendly. * Remove unused comment * Eliminate the promiseType variable This variable is only used once. We should not define it here. * Eliminate double try/catch blocks The nested try/catch blocks are not necessary. The finally block is going to run even if it returns an error in the catch block. * Add comments to describe what T is Need to make sure the generic type is clear. * Use wrapped promise The wrapped promise and runExclusive should be used instead of releasing the mutex manually. This is safer. * Change name to callback instead Name the variable to match the parameter name being passed into runExclusive. * Eliminate extra variable Use the async function in place. Don’t define an extra variable just for one usage. * Add a test for commit first If commit is first then make sure the calls happen in the right order. * Remove a redundant check for transaction started The results won’t be any different if the first check is removed. Therefore, should remove it just because it makes the code a lot simpler. * Rename variable and use runExclusive runExclusive in the run function will get rid of excess code that is not needed. Also rename a variable to runResults to be more specific. * refactor: remove resolvers, remove promise in withBeginTransaction, simplify code calling withBeginTransaction (#1212) * Change resolvers Change the resolvers so that they all follow the same pattern. * Add a wrapWithBeginTransaction function for get Generalize the calls to withBeginTransaction. Don’t allow the error type to be undefined in UserCallbackData since the error never actually will be undefined. This is necessary to solve various compile time errors. * Use wrapWithBeginTransaction in more places commit and runQuery can now use #wrapWithBeginTransaction and this will reduce the amount of code necessary in each function respectively. * Add callbackWithError function This function replaces a common pattern that occurs in transactions where we provide a callback that accepts an error as the first argument and some generic type that extends an array of any as the rest of the arguments. * Add return type to callbackWithError Adding a return type just makes type checking stronger in the client library. We should introduce it here to make it more likely a compiler error will be thrown if the function is misused. * Delete code that is not used When playing around with code to return a standard callback, code was written that is not required anymore. Also, rename StandardCallbackArgumentsAny. * There is no need to define a separate type The UserCallbackArguments do not need a separate type if they are only used once. Define the type inline where it is needed. * TODO no longer applies * Remove TODO * Inline the error type Error or null is only used in two places so inline it here. * Add a comment for callbackWithError callbackWithError is an abstract function that needs to be explained. This comment helps with that. * Added comments to document sendUserCallbackData sendUserCallbackData needs to be described so that its purpose is clear. * Move function Move function to the right order alphabetically * Do not define a commit type only used once Do not define a separate variable for only one use. * Replace the type in the comment with Function Replace with Function to eliminate references to the generic parameter which make CI tests fail. * Just use function type for the callback No need to mention the argument types and return types. * Remove withBeginTransaction withBeginTransaction is only called by one function so remove it to condense call stack. * Simplify code usage of withBeginTransaction Eliminate the need to build a resolver for all code usages of withBeginTransaction. Eliminate the need for a promise in withBeginTransaction. Eliminate the need for a callbackWithError function. * function should not have any arguments * Remove generic Args parameter It is not used. * Remove TODOs that are not relevant anymore. * Rename to withBeginTransaction Rename the function and eliminate redundant code. * Update comments for withBeginTransaction Update the comments to reflect new parameters. * Simplify diff * Remove error as null * withBeginTransaction Move withBeginTransaction to last alphabetically * Add a comment to indicate error In the code block it is not clear what kind of error will be produced and caught in catch so a comment is added to clarify the type of error that gets produced. * Rename method to beginTxAsync Calling the method `run` is too ambiguous. Use #beginTransactionAsync instead. * Remove abstract types and inline instead With changes from before, these abstract data types are now only used once in the async function that begins a transaction. This change inlines the code that creates the promise so that these don’t have to be used at all. * Change the type of the id The id for a transaction should actually match the value returned by the server. Adding this change also allows us to be more specific with types in function arguments. * For types to align, previous txn must be flexible Previous transaction must accept the Uint8Array type because after all, that is what the id type usually is. * Set transaction type in request options The request options must accept a Uint8Array for the compiler to work now that the ids can accept this type too. * For read options, more types should be accepted The type for read options should line up with the type for other properties that store a transaction id. * Replace UserCallbackData with BeginAsyncResponse UserCallbackData is only used in one place. Therefore, the generic type is not necessary and this can be replaced with a more specific type for readability. * Rewrite description for withBeginTransaction. This description adds a few more details. * Add comments to withBeginTransaction Ensure that the use of withBeginTransaction is clear and that this function contains comments throughout it. * Make the MockedTransactionWrapper class flexible We want to test what happens when sending an error back from the begin transaction call. * Move the after hook out The after hook does the same thing for all four of the describe blocks. Move it out to reduce the amount of code required. * Add beginTransaction error test Add a test to ensure that when beginTransaction sends back an error that the error actually reaches the user when they are using a promise or a callback. * Add comments to withBeginTransaction Comments are needed in withBeginTransaction to clearly indicate the flow of control within the function. * Added a few comments for withBeginTxn Comments will help all functions that are using withBeginTransaction. --------- Co-authored-by: Mend Renovate <renovate@whitesourcesoftware.com> Co-authored-by: gcf-owl-bot[bot] <78513119+gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
](https://renovatebot.com){kind=link}
danieljbruce
added a commit
that referenced
this pull request
Feb 26, 2024
* refactor: Break transaction.run into smaller pieces for use with async functions and other read/write calls (#1196) * Add a unit test for commit as a non-tx The fact that using a transaction object to do a commit results in a non-transaction should be documented so that if we decide to introduce a change later where this behaves differently then it is well documented. # Conflicts: # test/transaction.ts * use linter * Write tests to capture current behavior of error When begin transaction sends back an error, we want some tests to capture what the behavior is so that when we make changes to the run function then behavior is preserved. * Add tests for passing a response A response should reach the user the right way. Add tests to make sure behavior is preserved. * run async close to working In the run function delegate calls to runAsync and use run async to make promise calls * Add runAsync to promise excludes This allows this function to return a promise instead of a promise wrapped in a promise. This makes the tests pass and behave the way they should. * Remove space * Change to use this instead of self Do not call request from self * Eliminate unused comments * Add two comments Comments should actually explain what is being done * Remove the commit test for this PR The commit test for this PR should be removed because it is not really relevant for the async run functionality. * Clarify types throughout the function The types used should be very specific so that reading the code isn’t confusing. * Add a bit more typing for clarity Add a type to the resolve function just to introduce more clarity * Change types used in the data client callback Make the types more specific in the data client callback so that it is easier to track down the signature and match against the begin transaction function. * run the linter * Add comments to clarify PR * Refactor the parsing logic out of run The parsing logic is going to be needed elsewhere so taking it apart now. * Change interface of request promise callback The interface name should be changed so that it matches what it is. It is the callback used to form a promise. * Hide data completely Change accessors to hide data completely instead of using the private modifier * PR use if/else block Eliminate the early return as suggested in the PR * Add comments to document the new functions The comments capture the parameters and return type. * Update return type in docs * Update the tests to include runAsync runAsync should be in promisfy excludes * refactor: Break transaction.run into smaller pieces for use with async functions and other read/write calls * Rename a function to be more descriptive Make sure it is explicit that we are parsing begin results. * Modify comment Modify comment so that it doesn’t reference the way the code was before. * refactor: Move commit logic and add tests that prepare for transaction function changes (#1202) * Add a unit test for commit as a non-tx The fact that using a transaction object to do a commit results in a non-transaction should be documented so that if we decide to introduce a change later where this behaves differently then it is well documented. # Conflicts: # test/transaction.ts * use linter * Write tests to capture current behavior of error When begin transaction sends back an error, we want some tests to capture what the behavior is so that when we make changes to the run function then behavior is preserved. * Add tests for passing a response A response should reach the user the right way. Add tests to make sure behavior is preserved. * run async close to working In the run function delegate calls to runAsync and use run async to make promise calls * Add runAsync to promise excludes This allows this function to return a promise instead of a promise wrapped in a promise. This makes the tests pass and behave the way they should. * Remove space * Change to use this instead of self Do not call request from self * Eliminate unused comments * Add two comments Comments should actually explain what is being done * Remove the commit test for this PR The commit test for this PR should be removed because it is not really relevant for the async run functionality. * Clarify types throughout the function The types used should be very specific so that reading the code isn’t confusing. * Add a bit more typing for clarity Add a type to the resolve function just to introduce more clarity * Change types used in the data client callback Make the types more specific in the data client callback so that it is easier to track down the signature and match against the begin transaction function. * run the linter * Add comments to clarify PR * Refactor the parsing logic out of run The parsing logic is going to be needed elsewhere so taking it apart now. * Change interface of request promise callback The interface name should be changed so that it matches what it is. It is the callback used to form a promise. * Move commit functionality to a new function The internals of commit should be moved to a new function. This way we can sandwich a commit async call between commit and runCommitAsync. * Add the commit tests Commit tests added to ensure that commit behaves the same way as before. * Fix the tests so that they pass on commit The tests should pass before we make changes to commit. * refactor one of the mocks One of the mocks does not need to be written twice * Hide data completely Change accessors to hide data completely instead of using the private modifier * PR use if/else block Eliminate the early return as suggested in the PR * Add comments to document the new functions The comments capture the parameters and return type. * Update return type in docs * Update the tests to include runAsync runAsync should be in promisfy excludes * refactor: Break transaction.run into smaller pieces for use with async functions and other read/write calls * Rename a function to be more descriptive Make sure it is explicit that we are parsing begin results. * chore(deps): update dependency @types/sinon to v17 (#1197) * chore(deps): update dependency @types/is to v0.0.25 (#1198) [](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@types/is](https://togithub.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/is) ([source](https://togithub.com/DefinitelyTyped/DefinitelyTyped)) | [`0.0.24` -> `0.0.25`](https://renovatebot.com/diffs/npm/@types%2fis/0.0.24/0.0.25) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: Branch creation - "after 9am and before 3pm" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/googleapis/nodejs-datastore). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40Ni4wIiwidXBkYXRlZEluVmVyIjoiMzcuNDYuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> * Modify comment Modify comment so that it doesn’t reference the way the code was before. * Create a transaction wrapper class for testing This is going to be a useful test for mocking out various layers to make sure they work the same way as before. * Clean up mocked transaction wrapper The mocked transaction wrapper should reset all mocked gapic functions and not have to be told which ones to reset. * Move test information for commit into commit block * Add gapic mocked tests for aggregation query Gapic mocked tests for run aggregation query need to be written that use the mock transaction object. * Fixing up the runQuery test * Finished the runQuery tests The runQuery tests are finished so we can use this to take apart the function from end to end. * Finished the get tests The get tests now make sure that the data coming back from the gapic layer results in the same values for users. * remove only * Add try catch blocks to handle errors in the tests try/catch logic in the test suite is added so that errors bubble up to the test runner and it is easier to see why tests failed * chore: update cloud-rad version to ^0.4.0 (#1199) Source-Link: googleapis/synthtool@1063ef3 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:e92044720ab3cb6984a70b0c6001081204375959ba3599ef6c42dd99a7783a67 Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * Add comments and general cleanup Cleanup includes adding comments so that the tests are more readable. * Remove some redundant tests Some of these tests are captured in the various functions describe block. * Correct the comment to be more accurate The comment should talk about what the tests actually intend to do * General improvements to code quality Move declared transactionWrapper and other variables out to shorten code. Remove require and replace with import. Use more specific type. * Add data client check Add data client check and add type for data client. * Eliminate the mocked function variable The mocked function variable is not used so remove it from code. * Move begin transaction setup code The same block of code for setting up begin transaction is repeated twice. Move it into one function and use it from both before blocks. * Replace the TODO for the key Add some try blocks to make the errors more visible also. * Update description Update the description for the test to give a better explanation of what the describe block does. * Add comments to describe the purpose of signaller The comments describing the callback signaller should explain the problem it solves. * Add both dones back in The done functions were lost in the refactors. Let us add them back in. * mockedBeginTransaction should be Function Make a more specific type for mockedBeginTransaction and the functions mocked. * Use ECMA script modifier Make sure this function is completely hidden. * Remove TODOs that no longer apply The TODO statements no longer need to be followed up on so remove them. * beginTransaction type definition Remove the type definition. It is not used. * Add the setupBeginTransaction method Refactor test code to provide better error catching and also not repeat setup code for the run function. * Add a comment for private runCommit method The comment for the runCommit method just captures the fact that the function is used as a pass-through. * Modify comment to be more clear * Update comments The comments should more directly address the problem the tests are meant to solve * Eliminate redundant test code This describe block is now duplicated. Remove it. * revert comment This was a typo * refactor: Move commit logic and add tests that prepare for transaction function changes --------- Co-authored-by: Mend Renovate <renovate@whitesourcesoftware.com> Co-authored-by: gcf-owl-bot[bot] <78513119+gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * feat: Begin transactions before each transaction read/write (#1205) * Add a unit test for commit as a non-tx The fact that using a transaction object to do a commit results in a non-transaction should be documented so that if we decide to introduce a change later where this behaves differently then it is well documented. # Conflicts: # test/transaction.ts * use linter * Write tests to capture current behavior of error When begin transaction sends back an error, we want some tests to capture what the behavior is so that when we make changes to the run function then behavior is preserved. * Add tests for passing a response A response should reach the user the right way. Add tests to make sure behavior is preserved. * run async close to working In the run function delegate calls to runAsync and use run async to make promise calls * Add runAsync to promise excludes This allows this function to return a promise instead of a promise wrapped in a promise. This makes the tests pass and behave the way they should. * Remove space * Change to use this instead of self Do not call request from self * Eliminate unused comments * Add two comments Comments should actually explain what is being done * Remove the commit test for this PR The commit test for this PR should be removed because it is not really relevant for the async run functionality. * Clarify types throughout the function The types used should be very specific so that reading the code isn’t confusing. * Add a bit more typing for clarity Add a type to the resolve function just to introduce more clarity * Change types used in the data client callback Make the types more specific in the data client callback so that it is easier to track down the signature and match against the begin transaction function. * run the linter * Add comments to clarify PR * Refactor the parsing logic out of run The parsing logic is going to be needed elsewhere so taking it apart now. * Change interface of request promise callback The interface name should be changed so that it matches what it is. It is the callback used to form a promise. * Move commit functionality to a new function The internals of commit should be moved to a new function. This way we can sandwich a commit async call between commit and runCommitAsync. * Add the mutex and transaction state etc. Still an issue with system tests. commit still needs tweaks to work with async. * Remove no-op, get commit tests working Remove the no-op, get commit tests in place. * Add mocks and additional debugging Mocks and additional debugging hooks to introspect what is going on. * Add tests for commit Make sure commit behaves the same way as before. * Add the commit tests Commit tests added to ensure that commit behaves the same way as before. * Fix the tests so that they pass on commit The tests should pass before we make changes to commit. * refactor one of the mocks One of the mocks does not need to be written twice * reverting changes to add new test on transaction * Change the promise Make the promise simpler. Change the tests to exclude functions with promisify. * Hide data completely Change accessors to hide data completely instead of using the private modifier * PR use if/else block Eliminate the early return as suggested in the PR * Add comments to document the new functions The comments capture the parameters and return type. * Update return type in docs * Update the tests to include runAsync runAsync should be in promisfy excludes * refactor: Break transaction.run into smaller pieces for use with async functions and other read/write calls * Rename a function to be more descriptive Make sure it is explicit that we are parsing begin results. * Move the mutex and the state down to derived class The mutex and state should be moved down to the derived class. We are going to override get/runQuery/runAggregateQuery there. * chore(deps): update dependency @types/sinon to v17 (#1197) * Add hook to call run before commit Add the hook to call run before calling commit in existing tests. * Add commitAsync to promisify excludes commitAsync should be resolved and then() function should be called as it was not being called before * remove the console logs * Delete run commit * Remove the private identifier Use the private modifier instead to hide data * Add withBeginTransaction withBeginTransaction will be used with all calls that begin transactions and then intend to use the mutex for locking when the begin transaction call is made. * Add another level of abstraction Add #beginWithCallback so that all the read calls can be made with one line of code. * commit async is not needed anymore * This data structure is not needed anymore * Replace types associated with run to use generics The generic parameter should be used for types with run. * Make response type more specific for parseRunAsync More specific types are better and it make code easier to read * chore(deps): update dependency @types/is to v0.0.25 (#1198) [](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@types/is](https://togithub.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/is) ([source](https://togithub.com/DefinitelyTyped/DefinitelyTyped)) | [`0.0.24` -> `0.0.25`](https://renovatebot.com/diffs/npm/@types%2fis/0.0.24/0.0.25) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: Branch creation - "after 9am and before 3pm" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/googleapis/nodejs-datastore). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40Ni4wIiwidXBkYXRlZEluVmVyIjoiMzcuNDYuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> * Modify comment Modify comment so that it doesn’t reference the way the code was before. * Add implementation for runQuery runQuery should make the call to begin the transaction first if that hasn’t already happened yet. * Making fixes for runAggregationQuery Fixes for run aggregation query. Still getting undefined results. * Write tests for runAggregateQuery Make sure that runAggregateQuery return results make it back to the user. * Get one test case passing for runAggregateQuery runAggregateQuery should not be excluded by promisify. Otherwise it will not return a promise, but we want it to return a promise. * remove console log clutter * Change tests for runAggregationQuery Change the test to use deep strict equal since the objects being compared will not be reference equal. * Add resolver type Eliminate some unused code * Create a transaction wrapper class for testing This is going to be a useful test for mocking out various layers to make sure they work the same way as before. * Clean up mocked transaction wrapper The mocked transaction wrapper should reset all mocked gapic functions and not have to be told which ones to reset. * Move test information for commit into commit block * Add gapic mocked tests for aggregation query Gapic mocked tests for run aggregation query need to be written that use the mock transaction object. * Fixing up the runQuery test * Finished the runQuery tests The runQuery tests are finished so we can use this to take apart the function from end to end. * Finished the get tests The get tests now make sure that the data coming back from the gapic layer results in the same values for users. * remove only * remove only and console log * Try modifications to runQuery Add modifications to runQuery to use some function to return values * Add try catch blocks to handle errors in the tests try/catch logic in the test suite is added so that errors bubble up to the test runner and it is easier to see why tests failed * Modify commit so it doesn’t run early Don’t start commit with a promise or the promise will run early. * Update get with resolver get should use the same pattern as runQuery and runAggregationQuery to use a resolver for the mutex business logic * remove #beginWithCallback #beginWithCallback is no longer used so remove it * Remove #withBeginTransaction This function is no longer used * Rename #someFunction with begin transaction #someFunction should be named differently * Fix promisify. Change to a deepStrictEqual check. A deepStrictEqual check is all that is needed in this test that currently reuses a transaction. * Add setImmediate to the tests If we add a delay to the tests then the mutex has the opportunity to unlock before running the tests. * Added some tests for call ordering We want to make sure that the calls get delivered and received in order * Pack testing tool into an object Put the testing tool in an object so that we can explore more orderings and possibilities. * Eliminate console logs, use expected order Allow expected order to be passed into the tester. This will make the object reusable. * Add a check for transaction not started yet Need a check to be sure that the transaction is not in progress if making another beginTransaction call. * Remove NOT_TRANSACTION Remove the NOT_TRANSACTION and default to NOT_STARTED * Remove only Remove only and let all unit tests run * Use an enum instead of static class members Use an enum to track transaction state. * TODOs are done * Move excludes to proper position Excludes should contain the right functions in the right order. * Simplify the run function a little bit Regroup functionality to simplify the run function * Add comments to tests Explain purpose of testing objects * Modify tests and fix a bug from the merge The merge added a bug because the pound sign wasn’t used and it should have been. * Fix error message * Comments and general cleanup Remove constructor parameter that is not used. Add comments to functions so that the tests are more readable. * Added comments for functions in transaction.ts Functions in transaction.ts need JSdoc comments. * Add a comment for aggregate queries Add JSdoc comment * Add an assertion to test The assertion should make sure the first read returns undefined. * Add tests for lookup, put, commit For calls with run and without run, do lookup, put, commit. Make sure to clean up after the tests too. * refactor the test Test does lookup, put, commit. This is done in an async function. * Add tests for put, lookup, commit A group of tests should be added for put, lookup, commit. One with run and one without run. * chore: update cloud-rad version to ^0.4.0 (#1199) Source-Link: googleapis/synthtool@1063ef3 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:e92044720ab3cb6984a70b0c6001081204375959ba3599ef6c42dd99a7783a67 Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * Add comments and general cleanup Cleanup includes adding comments so that the tests are more readable. * Remove some redundant tests Some of these tests are captured in the various functions describe block. * Correct the comment to be more accurate The comment should talk about what the tests actually intend to do * General improvements to code quality Move declared transactionWrapper and other variables out to shorten code. Remove require and replace with import. Use more specific type. * Add data client check Add data client check and add type for data client. * Eliminate the mocked function variable The mocked function variable is not used so remove it from code. * Move begin transaction setup code The same block of code for setting up begin transaction is repeated twice. Move it into one function and use it from both before blocks. * Replace the TODO for the key Add some try blocks to make the errors more visible also. * Update description Update the description for the test to give a better explanation of what the describe block does. * Add comments to describe the purpose of signaller The comments describing the callback signaller should explain the problem it solves. * Add both dones back in The done functions were lost in the refactors. Let us add them back in. * mockedBeginTransaction should be Function Make a more specific type for mockedBeginTransaction and the functions mocked. * Use ECMA script modifier Make sure this function is completely hidden. * Remove TODOs that no longer apply The TODO statements no longer need to be followed up on so remove them. * beginTransaction type definition Remove the type definition. It is not used. * Add the setupBeginTransaction method Refactor test code to provide better error catching and also not repeat setup code for the run function. * Add a comment for private runCommit method The comment for the runCommit method just captures the fact that the function is used as a pass-through. * Modify comment to be more clear * Update comments The comments should more directly address the problem the tests are meant to solve * Eliminate redundant test code This describe block is now duplicated. Remove it. * revert comment This was a typo * refactor: Move commit logic and add tests that prepare for transaction function changes * runQuery, put, commit Another some more integration tests for runQuery, put, commit * run linter and group some tests into a describe block so that they don’t run before some new tests we are going to add. * Add some runAggregationQuery integration tests Add one of the test cases from the document (runAggregationQuery tests) * Add tests for put, runAggregationQuery, commit put, runAggregationQuery, commit tests have been added and now run properly. * Write some latency tests The latency tests measure time taken with and without using the run call. * Add logs to make latency tests run The logs will output the time required for the latency tests. * Add two tests for put, commit The two tests for put, commit should make sure that begin transaction is called. * Build requests into the transaction order tester To meet the needs we want for unit testing we must add some checker that lets us record the requests and verify that they are the right values. * Modify order testing Modify the order tester object to include more calls. Also add expected requests to the existing tests. * Create lookup, lookup, put, commit Create test for a read, read and then commit. * Fix the unit test Unit test should capture the fact that the run callback is used. * Add lookup requests When using get requests without passing consistency, we should see two lookup requests reach the gapic layer. Consistency should be removed from the test because it is not meant to reach the Gapic layer. * Remove the console logs console logs are not needed. Remove them so that the tests are cleaner. * Remove only * remove the latency tests The latency tests are not needed anymore so remove them. * Separate into a section transactions with/without A section name transactions with and without run should be used. * Remove a describe block and run linter This should make the diff a lot cleaner * Re-introduce a test that was there from before We don’t want the diff to say that we have removed tests. Therefore, add the test that was there before. * Update comment Modify the comment so that it is more objective * Add a comment for the runAggregationQuery function The comment for the aggregation query function in transaction.ts is needed here. * Simplify argument type In #withBeginTransaction there is a data type that matches the complex data type in the second argument of this function. * remove unnecessary extra variable The extra variable is not needed * Eliminate redundant definition Eliminate a definition and use the commit response data type here instead. * Add a comment for the #withBeginTransaction fn Need to clearly explain what the withBeginTransactionFunction does * Eliminate a line that was used for debugging * Add space back in * remove indent * Add empty line back in This simplifies the diff a bit * Remove ambiguous pronoun * Shorten comment * Add a comment for begin Add a comment that incorporates how this function interacts with #withBeginTransaction. * Shorten comments Don’t repeat information from the comments in the super class. Just mention the super class comments. * Type is only used once Inline the type to eliminate redundancy in the code. * Inline another promise type Inline another promise type which lets us eliminate one line of code. * Rename type to executor Executor is what it is called in the promise argument. * run linter * Simplify the data types more The types should line up for the executor in the runAsync function * Rename type to describe how it is used This name is clearer * Rename to userCallbackData. Add comments UserCallbackData is a more specific description of the type of data the generic type is. We also need comments to explain what these interfaces should represent. * resolver is a better term to use here * Change the type in the comments The type has updated since to be a more general type. * Flip if/else Flip the if else. Improvements are that the if checks for the affirmative and the longer block comes first * run linter * Add brackets Introduce brackets so that the word matching the parameter is more isolated. * Stronger type checking for runAsync Use more specific types in the runAsync function so that the compiler will complain if anything isn’t right. * Call the function fed into the promise resolver resolver should be the name used just to be consistent with all other code. * Rename variables and add comments Rename the variables so that the relationships between them are easier to understand. Add comments explaining the purpose of the variables. * Add comment for the done function * Make checkForCompletion private To avoid confusion make checkForCompletion private so that the user of the object does not need to think about using it. * Replace all events with enum values The events should not be strings. If they are strings then we might be magic string matching and this should not be done in code. * Make code more succinct Remove the need for a map. Remove occurrences of strings throughout tests and replace them with enum values. * Rename event to UserCodeEvent Call this enum UserCodeEvent as it is a better description of what the values could potentially be. * Remove the map from string names to gapic layer This map is not needed anymore because now we use the Gapic layer function event directly. * More specific type Shouldn’t just be a string. This should be a Gapic Layer function. * todo is done Assertion check is done * Eliminate need to define extra variable This just makes the code a bit more concise. * Add comments to describe test objects The comments describe Gapic layer data passed back. * There is no need to make these functions private Just inline the callbacks. A private function makes their relationship with the code that uses them confusing and each one of them is only used in one place. * Renames all callbacks to just callback Rename all callbacks to callback because that is the variable name inside the client function. * Take requests private * Take expectedRequests private expectedRequests is not used outside of the transaction order tester so we should take it private. * Take expected event order private Expected event order should be taken private. * Take event order private event order is not used outside this object * Fix some warning messages Various warnings show up in the es-lint. Fix them. * Use unknown for Gapic Layer Response Change the type to unknown to address the ES-lint error. * Get rid of any type for runQuery Use the more specific type provided by runQuery instead. * More linter fixes Addressing some of the problems in the new tests according to ES-lint. * Change parameters to match the get callback Stronger type enforcement helping us catch errors later. * Another ESLInt correction Use GetCallback here instead of any for the Get response type. * Remove query variable as it is not used * Take done private When done is passed in and saved it should only be used internally. * Eliminate unused arguments in callback Eliminate the arguments because if they are not used then they cause ES-lint errors. * Add readOnly reserved word Various properties can be readOnly as recommended by the linter. * Make type for done explicit Explicitly call out the fact that done should equal mocha’s done function. * Another place where done should be mentioned The done type should explicitly be mentioned here. * No need to access order tester to get wrapper There is no need to access the transaction wrapper to get the order tester. Just access the transaction wrapper directly. * private variable transaction wrapper Take transaction wrapper private. It does not need to be visible to the user. * Capitalize RequestType Change RequestType to be capitalized and use it whenever a request needs to be made. * request should be a more specific type RequestType should be used here. * Do info checks for error and response cases Add another assertion check. Do the check for the error and response cases. * These are actually results from runQuery * Eliminate extra line of code Add assertion check for info. Eliminate the code that stores all the results. * rename enum The word Layer is redundant. Also emphasize that the enum captures the function name. * Lookup response The variable should include the word lookup since technically that is the name of the Gapic layer endpoint. * Add comments for the transaction order tester Explain each function with a comment describing what it does. * Introduce a new push function Add a push function so that in each test it is easier to see the events line up. * Remove middle layer functions Remove functions in the transactionOrderTester and use functionality from those functions directly because then in the test it is easier to read the test’s intent. Also, add GapicRequestData type to simplify types in the test class. Shorten transactionOrderTester to just tester. * Prefer block comments The comments are multi-line so use block comments instead. * Rename event to CUSTOM_EVENT CUSTOM_EVENT should be used instead of FUNCTIONS_CALLED. FUNCTIONS_CALLED is sort of ambiguous. * Add square brackets to comments square brackets added to make the comment more readable and user friendly. * Remove unused comment * Eliminate the promiseType variable This variable is only used once. We should not define it here. * Eliminate double try/catch blocks The nested try/catch blocks are not necessary. The finally block is going to run even if it returns an error in the catch block. * Add comments to describe what T is Need to make sure the generic type is clear. * Use wrapped promise The wrapped promise and runExclusive should be used instead of releasing the mutex manually. This is safer. * Change name to callback instead Name the variable to match the parameter name being passed into runExclusive. * Eliminate extra variable Use the async function in place. Don’t define an extra variable just for one usage. * Add a test for commit first If commit is first then make sure the calls happen in the right order. * Remove a redundant check for transaction started The results won’t be any different if the first check is removed. Therefore, should remove it just because it makes the code a lot simpler. * Rename variable and use runExclusive runExclusive in the run function will get rid of excess code that is not needed. Also rename a variable to runResults to be more specific. * refactor: remove resolvers, remove promise in withBeginTransaction, simplify code calling withBeginTransaction (#1212) * Change resolvers Change the resolvers so that they all follow the same pattern. * Add a wrapWithBeginTransaction function for get Generalize the calls to withBeginTransaction. Don’t allow the error type to be undefined in UserCallbackData since the error never actually will be undefined. This is necessary to solve various compile time errors. * Use wrapWithBeginTransaction in more places commit and runQuery can now use #wrapWithBeginTransaction and this will reduce the amount of code necessary in each function respectively. * Add callbackWithError function This function replaces a common pattern that occurs in transactions where we provide a callback that accepts an error as the first argument and some generic type that extends an array of any as the rest of the arguments. * Add return type to callbackWithError Adding a return type just makes type checking stronger in the client library. We should introduce it here to make it more likely a compiler error will be thrown if the function is misused. * Delete code that is not used When playing around with code to return a standard callback, code was written that is not required anymore. Also, rename StandardCallbackArgumentsAny. * There is no need to define a separate type The UserCallbackArguments do not need a separate type if they are only used once. Define the type inline where it is needed. * TODO no longer applies * Remove TODO * Inline the error type Error or null is only used in two places so inline it here. * Add a comment for callbackWithError callbackWithError is an abstract function that needs to be explained. This comment helps with that. * Added comments to document sendUserCallbackData sendUserCallbackData needs to be described so that its purpose is clear. * Move function Move function to the right order alphabetically * Do not define a commit type only used once Do not define a separate variable for only one use. * Replace the type in the comment with Function Replace with Function to eliminate references to the generic parameter which make CI tests fail. * Just use function type for the callback No need to mention the argument types and return types. * Remove withBeginTransaction withBeginTransaction is only called by one function so remove it to condense call stack. * Simplify code usage of withBeginTransaction Eliminate the need to build a resolver for all code usages of withBeginTransaction. Eliminate the need for a promise in withBeginTransaction. Eliminate the need for a callbackWithError function. * function should not have any arguments * Remove generic Args parameter It is not used. * Remove TODOs that are not relevant anymore. * Rename to withBeginTransaction Rename the function and eliminate redundant code. * Update comments for withBeginTransaction Update the comments to reflect new parameters. * Simplify diff * Remove error as null * withBeginTransaction Move withBeginTransaction to last alphabetically * Add a comment to indicate error In the code block it is not clear what kind of error will be produced and caught in catch so a comment is added to clarify the type of error that gets produced. * Rename method to beginTxAsync Calling the method `run` is too ambiguous. Use #beginTransactionAsync instead. * Remove abstract types and inline instead With changes from before, these abstract data types are now only used once in the async function that begins a transaction. This change inlines the code that creates the promise so that these don’t have to be used at all. * Change the type of the id The id for a transaction should actually match the value returned by the server. Adding this change also allows us to be more specific with types in function arguments. * For types to align, previous txn must be flexible Previous transaction must accept the Uint8Array type because after all, that is what the id type usually is. * Set transaction type in request options The request options must accept a Uint8Array for the compiler to work now that the ids can accept this type too. * For read options, more types should be accepted The type for read options should line up with the type for other properties that store a transaction id. * Replace UserCallbackData with BeginAsyncResponse UserCallbackData is only used in one place. Therefore, the generic type is not necessary and this can be replaced with a more specific type for readability. * Rewrite description for withBeginTransaction. This description adds a few more details. * Add comments to withBeginTransaction Ensure that the use of withBeginTransaction is clear and that this function contains comments throughout it. * Make the MockedTransactionWrapper class flexible We want to test what happens when sending an error back from the begin transaction call. * Move the after hook out The after hook does the same thing for all four of the describe blocks. Move it out to reduce the amount of code required. * Add beginTransaction error test Add a test to ensure that when beginTransaction sends back an error that the error actually reaches the user when they are using a promise or a callback. * Add comments to withBeginTransaction Comments are needed in withBeginTransaction to clearly indicate the flow of control within the function. * Added a few comments for withBeginTxn Comments will help all functions that are using withBeginTransaction. --------- Co-authored-by: Mend Renovate <renovate@whitesourcesoftware.com> Co-authored-by: gcf-owl-bot[bot] <78513119+gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Mend Renovate <renovate@whitesourcesoftware.com> Co-authored-by: gcf-owl-bot[bot] <78513119+gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes:
Modifies withBeginTransaction so that it doesn't await a promise anymore. Instead, it just calls the function that runs the code we want to run after ensuring the transaction has been started. Since the promise resolved to a UserCallbackData type, this type no longer needs to be used in this function because the promise isn't there anymore. Instead of relying on a value returned by withBeginTransaction and parsing that into callback arguments, withBeginTransaction just calls the callback directly which eliminates another layer in the call stack.
Code calling withBeginTransaction also doesn't use resolvers anymore and logic parsing the return value of withBeginTransaction isn't needed anymore either. This dramatically simplifies code necessary for all functions calling withBeginTransaction.