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

Mvom increment #750

Merged
merged 25 commits into from
Aug 6, 2024
Merged

Mvom increment #750

merged 25 commits into from
Aug 6, 2024

Conversation

philfuster
Copy link
Contributor

@philfuster philfuster commented Aug 5, 2024

Summary

This PR implements the new increment operation.

TypeScript Changes

  • Created Input and Output types for new increment operation
  • Created async static method increment on dynamically generated Model class via compileModel.
  • Created transformPathToOrdinalPosition to transform an increment operation input key path to its ordinal position.
  • Updated Connection to handle new RecordNotFoundError.

UniBasic Changes

  • Created unibasicTemplate - increment.njk
    • validates increment operation input
    • Has a read lock loop to handle locked records.
      • sleeps according to the passed in retry delay.
      • returns RecordLockedError when retry max is exceeded
    • returns new RecordNotFoundError error when else statement executes on readu.
    • writes record after performing incrementing.
    • returns the updated document

Testing Notes

Ran tests using MVOM:

  • incrementing a record that does not exist - received RecordNotFoundError
  • incrementing one field - field was incremented, updated document returned
  • incremented multiple fields - fields were incremented, updated document returned
  • incremented nested field - nested field was incremented, updated document returned
  • incremented value position - value position was incremented
  • incremented multiple value positions - value positions were incremented
  • incremented sub value positions - sub value positions were incremented
  • incremented while record was locked - retries were performed - RecordLockedError returned.

@philfuster philfuster marked this pull request as draft August 5, 2024 23:31
@philfuster philfuster marked this pull request as ready for review August 5, 2024 23:39
Copy link
Member

@shawnmcknight shawnmcknight left a comment

Choose a reason for hiding this comment

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

I've finished a first pass review and left several comments. Overall this looks good, but I had suggestions for improvement.

src/compileModel.ts Outdated Show resolved Hide resolved
src/compileModel.ts Outdated Show resolved Hide resolved
src/compileModel.ts Outdated Show resolved Hide resolved
src/types/DbSubroutineOutput.ts Outdated Show resolved Hide resolved
src/compileModel.ts Outdated Show resolved Hide resolved
src/unibasicTemplates/subroutines/external/increment.njk Outdated Show resolved Hide resolved
src/Schema.ts Outdated Show resolved Hide resolved
src/Schema.ts Outdated Show resolved Hide resolved
@philfuster philfuster force-pushed the mvom_increment branch 3 times, most recently from 418e62c to 752c858 Compare August 6, 2024 20:04
@philfuster
Copy link
Contributor Author

I've finished a first pass review and left several comments. Overall this looks good, but I had suggestions for improvement.

@shawnmcknight Thank you for your review. I have addressed your feedback in the latest series of commits. This is ready for another look.

switching to setting up the TS code so I can deploy and test.
- remove unnecessary continue
- fixed indentation in increment and save when creating null projection
- added pattern match test
…ill not return null data.

update the DbSubroutineOutputIncrement type.
update tests.
…ccept the key paths.

Throw an error when invalid path encountered and when paths length is 0 because returning anything else would result in bad data when the U2 code processes the paths.
Consolidate buildOrdinalPaths within transformPathsToOrdinalPositions by making the logic simpler.
Update tests
…ition. Its only use was for transforming a single path so no point in having it work for multiple and having to work around that.

Updated tests accordingly.
Updated compileModel to use it as necessary
Copy link
Member

@shawnmcknight shawnmcknight left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few minor comments left.

src/Schema.ts Outdated Show resolved Hide resolved
src/Schema.ts Outdated Show resolved Hide resolved
src/unibasicTemplates/subroutines/external/increment.njk Outdated Show resolved Hide resolved
src/compileModel.ts Outdated Show resolved Hide resolved
src/__tests__/Schema.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@shawnmcknight shawnmcknight left a comment

Choose a reason for hiding this comment

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

LGTM!

@shawnmcknight shawnmcknight merged commit d035344 into main Aug 6, 2024
4 checks passed
@shawnmcknight shawnmcknight deleted the mvom_increment branch August 6, 2024 22:39
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