Skip to content

Conversation

@frolvanya
Copy link
Collaborator

This PR introduces additional validation checks for certain configuration variables. Specifically, it ensures that all URLs and f64 values adhere to their expected formats and constraints

Here's an example of running rpc-server with invalid env variables:
image

@frolvanya
Copy link
Collaborator Author

@race-of-sloths please, include my PR in the Race

@frolvanya frolvanya requested review from khorolets and kobayurii July 16, 2024 17:38
@race-of-sloths
Copy link

race-of-sloths commented Jul 16, 2024

@frolvanya Thank you for your contribution! Your pull request is now a part of the Race of Sloths!
New Sloth joined the Race! Welcome!

Shows profile picture for the author of the PR

Current status: executed
Reviewer Score
@khorolets 2

The average score is 2

@frolvanya check out your results on the Race of Sloths Leaderboard! and in the profile

What is the Race of Sloths

Race of Sloths is a friendly competition where you can participate in challenges and compete with other open-source contributors within your normal workflow

For contributors:

  • Tag @race-of-sloths inside your pull requests
  • Wait for the maintainer to review and score your pull request
  • Check out your position in the Leaderboard
  • Keep weekly and monthly streaks to reach higher positions
  • Boast your contributions with a dynamic picture of your Profile

For maintainers:

  • Score pull requests that participate in the Race of Sloths
  • Engage contributors with fair scoring and fast responses so they keep their streaks
  • Promote the Race to the point where the Race starts promoting you
  • Grow the community of your contributors

Feel free to check our website for additional details!

Bot commands
  • For contributors
    • Include a PR: @race-of-sloths include to enter the Race with your PR
  • For maintainers:
    • Assign points: @race-of-sloths score [1/2/3/5/8/13] to award points based on your assessment.
    • Reject this PR: @race-of-sloths exclude to send this PR back to the drawing board.
    • Exclude repo: @race-of-sloths pause to stop bot activity in this repo until @race-of-sloths unpause command is called

Copy link
Member

@kobayurii kobayurii left a comment

Choose a reason for hiding this comment

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

I don't think it's the best solution to write new functions for field deserialization and validation every time. Please consider using a third-party crate, for example serde_valid.

@frolvanya
Copy link
Collaborator Author

frolvanya commented Jul 16, 2024

I don't think it's the best solution to write new functions for field deserialization and validation every time. Please consider using a third-party crate, for example serde_valid.

@kobayurii Yep, I thought about this and I'm also not a big fan of code duplication. However, I faced few issues, maybe I'm doing something wrong. The #[validate] attribute from serde_valid is designed to validate fields during serialization and deserialization. However, it is applied directly to fields that are deserialized without custom deserialization functions (deserialize_with). When you use #[serde(deserialize_with)], you're bypassing the default deserialization behavior, which includes the #[validate] checks. Unfortunately, we can't forget about deserialize_with and just stick with normal deserialization + validation workflow, because we still need to check whether we received a value from ENV or from config file

P.S: or we need to come up with the custom implementation of validation function and we'll basically end up with the same state of code + a little overhead, because we used an extra crate

P.S2: + we also need to come up with a custom validation for URLs, because basic #[validate] supports only min/max + regex and I don't want to use pure regex to verify a URL

@kobayurii
Copy link
Member

kobayurii commented Jul 16, 2024

I don't think it's the best solution to write new functions for field deserialization and validation every time. Please consider using a third-party crate, for example serde_valid.

@kobayurii Yep, I thought about this and I'm also not a big fan of code duplication. However, I faced few issues, maybe I'm doing something wrong. The #[validate] attribute from serde_valid is designed to validate fields during serialization and deserialization. However, it is applied directly to fields that are deserialized without custom deserialization functions (deserialize_with). When you use #[serde(deserialize_with)], you're bypassing the default deserialization behavior, which includes the #[validate] checks. Unfortunately, we can't forget about deserialize_with and just stick with normal deserialization + validation workflow, because we still need to check whether we received a value from ENV or from config file

P.S: or we need to come up with the custom implementation of validation function and we'll basically end up with the same state of code + a little overhead, because we used an extra crate

P.S2: + we also need to come up with a custom validation for URLs, because basic #[validate] supports only min/max + regex and I don't want to use pure regex to verify a URL

I gave you serde_valid as an example. You can choose any other crate that suits us. How about https://docs.rs/validator/latest/validator/. I think it might work for us.

@frolvanya
Copy link
Collaborator Author

I don't think it's the best solution to write new functions for field deserialization and validation every time. Please consider using a third-party crate, for example serde_valid.

@kobayurii Yep, I thought about this and I'm also not a big fan of code duplication. However, I faced few issues, maybe I'm doing something wrong. The #[validate] attribute from serde_valid is designed to validate fields during serialization and deserialization. However, it is applied directly to fields that are deserialized without custom deserialization functions (deserialize_with). When you use #[serde(deserialize_with)], you're bypassing the default deserialization behavior, which includes the #[validate] checks. Unfortunately, we can't forget about deserialize_with and just stick with normal deserialization + validation workflow, because we still need to check whether we received a value from ENV or from config file
P.S: or we need to come up with the custom implementation of validation function and we'll basically end up with the same state of code + a little overhead, because we used an extra crate
P.S2: + we also need to come up with a custom validation for URLs, because basic #[validate] supports only min/max + regex and I don't want to use pure regex to verify a URL

I gave you serde_valid as an example. You can choose any other crate that suits us. How about https://docs.rs/validator/latest/validator/. I think it might work for us.

Oh, I knew only about serde_valid and I didn't know about validator crate. It might work. Thanks, I'll try to refactor current code

@frolvanya
Copy link
Collaborator Author

frolvanya commented Jul 16, 2024

I rewrote validation to use validator crate, but I have few more questions:

  1. Is it better to panic if we encounter such config errors or simply send a warn message to logs as we do now? Some of the checks are pretty dangerous (again, shadow_consistency_rate might result to the whole rpc failure), so I'm thinking about throwing a tracing::error! along with panic
  2. Right now the log message is pretty unreadable one-liner (because maintainer didn't merge this PR for some reason Improve Display Impl - Each ValidationError should be on its own line Keats/validator#235) and I'm not sure if I should invest some time to make it prettier. The main difficulty is a complexity of ValidationErrorsKind structure (it is a enum with multiple different fields). Tell me if this is a good idea to think about the method to improve current log message. It's not hard, but I have to implement rewrite their implementation of fmt function
image image

P.S: @kobayurii Thanks for sharing such powerful crate with me! It might become handy in my pet projects

@frolvanya frolvanya requested a review from kobayurii July 16, 2024 21:34
@khorolets
Copy link
Member

@race-of-sloths score 2

@frolvanya

  1. Is it better to panic if we encounter such config errors or simply send a warn message to logs as we do now?

In such dangerous situations, I'd prefer to panic.

As for the second part of your question, let's keep "pretty unreadable" for now. We'll get back to it if we realize it's a real issue for us.

@race-of-sloths
Copy link

🌟 Score recorded!

@khorolets, thank you for scoring this pull request in the Race of Sloths!

Copy link
Member

@khorolets khorolets left a comment

Choose a reason for hiding this comment

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

Great job, I left a few minor comments and approve 👍

Copy link
Member

@kobayurii kobayurii left a comment

Choose a reason for hiding this comment

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

Great! I left a one comment and approve 👍

@frolvanya
Copy link
Collaborator Author

I've fixed everything. As I see both of you approved the PR, so I'll merge it

@frolvanya frolvanya merged commit 22dce2c into near:database-new Jul 17, 2024
@race-of-sloths
Copy link

✅ PR is finalized!

Your contribution is much appreciated with a final score of 2!
You have received 30 (20 base + 10 weekly bonus) Sloth points for this contribution

Another weekly streak completed, well done @frolvanya! To keep your weekly streak and get another bonus make pull request next week! Looking forward to see you in race-of-sloths

kobayurii pushed a commit that referenced this pull request Aug 7, 2024
* feat: added checks for config variables

* refactor: used `validator` crate for config validation

* chore: simplified formatting

* chore: reordered imports

* feat: panic on error instead of logging

* chore: provided better message error
kobayurii pushed a commit that referenced this pull request Aug 7, 2024
* feat: added checks for config variables

* refactor: used `validator` crate for config validation

* chore: simplified formatting

* chore: reordered imports

* feat: panic on error instead of logging

* chore: provided better message error
kobayurii pushed a commit that referenced this pull request Aug 7, 2024
* feat: added checks for config variables

* refactor: used `validator` crate for config validation

* chore: simplified formatting

* chore: reordered imports

* feat: panic on error instead of logging

* chore: provided better message error
kobayurii pushed a commit that referenced this pull request Aug 8, 2024
* feat: added checks for config variables

* refactor: used `validator` crate for config validation

* chore: simplified formatting

* chore: reordered imports

* feat: panic on error instead of logging

* chore: provided better message error
kobayurii pushed a commit that referenced this pull request Aug 8, 2024
* feat: added checks for config variables

* refactor: used `validator` crate for config validation

* chore: simplified formatting

* chore: reordered imports

* feat: panic on error instead of logging

* chore: provided better message error
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.

4 participants