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

feat: add validation to JSON transformer #830

Merged

Conversation

morrieinmaas
Copy link
Contributor

@morrieinmaas morrieinmaas commented Jun 2, 2022

closes #742
Signed-off-by: Moriarty [email protected]

@morrieinmaas morrieinmaas requested a review from a team as a code owner June 2, 2022 13:23
@morrieinmaas
Copy link
Contributor Author

Thanks for all the feedback!

Moriarty added 2 commits June 3, 2022 11:54
needs a little bit of work still - not all test pass

Signed-off-by: Moriarty <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022

Codecov Report

Merging #830 (ff330a2) into main (1f8b6ab) will decrease coverage by 0.00%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##             main     #830      +/-   ##
==========================================
- Coverage   87.57%   87.57%   -0.01%     
==========================================
  Files         463      465       +2     
  Lines       11083    11097      +14     
  Branches     1745     1753       +8     
==========================================
+ Hits         9706     9718      +12     
- Misses       1377     1379       +2     
Impacted Files Coverage Δ
.../modules/connections/services/ConnectionService.ts 87.14% <ø> (+0.19%) ⬆️
...ore/src/modules/dids/methods/web/WebDidResolver.ts 60.00% <ø> (+1.17%) ⬆️
packages/core/src/error/ClassValidationError.ts 75.00% <75.00%> (ø)
packages/core/src/utils/MessageValidator.ts 90.00% <88.88%> (-10.00%) ⬇️
packages/core/src/utils/JsonTransformer.ts 94.73% <90.90%> (-5.27%) ⬇️
packages/core/src/agent/Agent.ts 95.89% <100.00%> (ø)
packages/core/src/agent/MessageReceiver.ts 80.76% <100.00%> (-0.54%) ⬇️
packages/core/src/agent/MessageSender.ts 84.61% <100.00%> (ø)
packages/core/src/error/ValidationErrorUtils.ts 100.00% <100.00%> (ø)
packages/core/src/error/index.ts 100.00% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f8b6ab...ff330a2. Read the comment docs.

@morrieinmaas morrieinmaas force-pushed the feat/json-tranform-validator branch from a3e547e to 19a8f48 Compare June 3, 2022 11:09
Moriarty added 2 commits June 3, 2022 14:01
@morrieinmaas morrieinmaas force-pushed the feat/json-tranform-validator branch from fa72646 to 29f4e6d Compare June 3, 2022 14:51
@morrieinmaas morrieinmaas changed the title WIP: feat: wip add validation to JSON transformer feat: wip add validation to JSON transformer Jun 3, 2022
@morrieinmaas morrieinmaas changed the title feat: wip add validation to JSON transformer feat: add validation to JSON transformer Jun 3, 2022
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

This is a great addition @morrieinmaas! Left some notes mainly on the disabling of validation. The goal to integrate validation with transformation is to get rid of these hidden errors, so would be great if we could fix as much validation errors as possible

packages/core/src/agent/MessageReceiver.ts Outdated Show resolved Hide resolved
packages/core/src/agent/MessageReceiver.ts Outdated Show resolved Hide resolved
packages/core/src/agent/__tests__/AgentMessage.test.ts Outdated Show resolved Hide resolved
packages/core/src/decorators/ack/AckDecorator.test.ts Outdated Show resolved Hide resolved
packages/core/src/utils/JsonTransformer.ts Outdated Show resolved Hide resolved
packages/core/src/utils/JsonTransformer.ts Outdated Show resolved Hide resolved
packages/core/src/utils/JsonTransformer.ts Outdated Show resolved Hide resolved
packages/core/src/utils/JsonTransformer.ts Outdated Show resolved Hide resolved
packages/core/src/utils/MessageValidator.ts Outdated Show resolved Hide resolved
@morrieinmaas
Copy link
Contributor Author

Thanls for all the feedback @TimoGlastra. sorry hadn't managed to write up some more context. From running the tests it became clear the the sync check method doesn't validate as reliably as the async one. That resulted in some tests failed as they were. Was sort of a catch 22 situation when writing this last Friday.

Also, I'll add more in detail to your comments above, but essentially I was a little unsure of how consistent with the previous implementation we want to keep it aka not introducing breaking changes. I'll have another fresh look. Maybe I was over-complicating things and missed the elegant way

@morrieinmaas
Copy link
Contributor Author

I'll have another go at it today to incorporate your suggestions

@TimoGlastra
Copy link
Contributor

I was a little unsure of how consistent with the previous implementation we want to keep it aka not introducing breaking changes

I think the previous implementation has some issues so it's fine to introduce some breaking changes if they improve behaviour

@morrieinmaas
Copy link
Contributor Author

Also random question - I've had some tests failing locally that then worked on remote due to snapshot obsolete. How can I fix this?

Moriarty added 4 commits June 6, 2022 21:40
move validation logic to MessageValidator

Signed-off-by: Moriarty <[email protected]>
Signed-off-by: Moriarty <[email protected]>
Signed-off-by: Moriarty <[email protected]>
@TimoGlastra
Copy link
Contributor

Also random question - I've had some tests failing locally that then worked on remote due to snapshot obsolete. How can I fix this?

Can you share the error log?

@morrieinmaas
Copy link
Contributor Author

here is the entire output of yarn test. Those tests don't fail on remote. Not sure what to do about obsolete snapshots

@TimoGlastra
Copy link
Contributor

I think your environement got corrupted. To clean you can do the following to delete all indy wallets:

rm -rf ~/.indy_client

And as you don't have a postgres database running locally you can run the tests with:

yarn test --testPathIgnorePatterns ./packages/core/tests/postgres.test.ts

Can you see if the errors still persist?

packages/core/src/agent/MessageReceiver.ts Outdated Show resolved Hide resolved
packages/core/src/agent/__tests__/AgentMessage.test.ts Outdated Show resolved Hide resolved
packages/core/src/decorators/ack/AckDecorator.test.ts Outdated Show resolved Hide resolved
packages/core/src/decorators/ack/AckDecorator.test.ts Outdated Show resolved Hide resolved
packages/core/src/decorators/ack/AckDecorator.test.ts Outdated Show resolved Hide resolved
} catch (error) {
expect(error).toMatchObject([
Copy link
Contributor

Choose a reason for hiding this comment

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

weird this is losing all this data. If you solve this one error, will it give a new error? (i.e. it will deliver them one by one instead)

packages/core/src/utils/JsonTransformer.ts Outdated Show resolved Hide resolved
packages/core/src/utils/__tests__/JsonTransformer.test.ts Outdated Show resolved Hide resolved
packages/core/src/utils/__tests__/MessageValidator.test.ts Outdated Show resolved Hide resolved
packages/core/src/utils/MessageValidator.ts Outdated Show resolved Hide resolved
@morrieinmaas morrieinmaas requested a review from TimoGlastra June 15, 2022 13:42
packages/core/src/agent/MessageReceiver.ts Outdated Show resolved Hide resolved
packages/core/src/utils/MessageValidator.ts Outdated Show resolved Hide resolved
packages/core/src/utils/MessageValidator.ts Outdated Show resolved Hide resolved
packages/core/src/agent/MessageSender.ts Outdated Show resolved Hide resolved
packages/core/src/utils/__tests__/MessageValidator.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

A lot of work went into this PR, great work @morrieinmaas!

@TimoGlastra
Copy link
Contributor

@blu3beri can you give a review to unblock or leave more feedback?

@morrieinmaas
Copy link
Contributor Author

A lot of work went into this PR, great work @morrieinmaas!

haha puuh that's come a long way. thanks for all the feedback and assistance.🙏

Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

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

Unblocked.

@TimoGlastra TimoGlastra enabled auto-merge (squash) June 20, 2022 12:05
@TimoGlastra TimoGlastra merged commit 5b9efe3 into openwallet-foundation:main Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add validation to JsonTransformer.fromJson()
5 participants