feature: implement basic querystring params validation (resolves #3055)

[qwerty541]

This is basic implementation of querystring params validation for stats card. I think we should provide user-friendly errors in case if wrong param type was passed. Currently it shows something like str.split is not a function.

@rickstaa @anuraghazra Let me know what do you think about it. I will implement same for other cards if you approve this pull request.

Looks like it also almost does not affect on performance: 160ms before changes and 172ms after.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
github-readme-stats	✅ Ready (Inspect)	Visit Preview	Dec 3, 2023 0:49am

[codecov[bot]]

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (80b2d23) 98.02% compared to head (d656449) 97.94%.

Files	Patch %	Lines
src/common/validate.js	93.75%	8 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3393      +/-   ##
==========================================
- Coverage   98.02%   97.94%   -0.08%     
==========================================
  Files          27       28       +1     
  Lines        6267     6414     +147     
  Branches      549      570      +21     
==========================================
+ Hits         6143     6282     +139     
- Misses        121      129       +8     
  Partials        3        3              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[rickstaa]

@qwerty541, I appreciate the pull request. I've asked @anuraghazra for a review, mainly because he recently commented on the rationale behind avoiding strict query parsing. You can find his insights here: https://github.com/anuraghazra/github-readme-stats/pull/2761#discussion_r1248874628.

[rickstaa]

@qwerty541, I appreciate the pull request. I've asked @anuraghazra for a review, mainly because he recently commented on the rationale behind avoiding strict query parsing. You can find his insights here: #2761 (comment).

@rickstaa It is the thing that i wanted to discuss about this pull request with you and @anuraghazra. As you see, currently e2e tests fails on this branch, it was because requested URL contains extra query parameter for bursting cache. I also remember that we recently removed count_private parameter and most likely a lot of people have not removed it from card URL. If the current strict check will be kept a lot of cards will be broken. I see two ways to solve it, first is reserving these query parameters, and second way is completely allowing to add extra parameters. I want to know which of them better in your opinion.

I would keep the current behavoir where we allow extra parameters, but let's see what @anuraghazra thinks 👍🏻.

[qwerty541]

@qwerty541, I appreciate the pull request. I've asked @anuraghazra for a review, mainly because he recently commented on the rationale behind avoiding strict query parsing. You can find his insights here: #2761 (comment).

@rickstaa It is the thing that i wanted to discuss about this pull request with you and @anuraghazra. As you see, currently e2e tests fails on this branch, it was because requested URL contains extra query parameter for bursting cache. I also remember that we recently removed count_private parameter and most likely a lot of people have not removed it from card URL. If the current strict check will be kept a lot of cards will be broken. I see two ways to solve it, first is reserving these query parameters, and second way is completely allowing to add extra parameters. I want to know which of them better in your opinion.

I would keep the current behavoir where we allow extra parameters, but let's see what @anuraghazra thinks 👍🏻.

After rethinking i came to the conclusion that there is no significant reasons to prohibit extra parameters. Are you up to approve this pull request for merging if i rework it this way?

[qwerty541]

@qwerty541, I appreciate the pull request. I've asked @anuraghazra for a review, mainly because he recently commented on the rationale behind avoiding strict query parsing. You can find his insights here: #2761 (comment).

@rickstaa It is the thing that i wanted to discuss about this pull request with you and @anuraghazra. As you see, currently e2e tests fails on this branch, it was because requested URL contains extra query parameter for bursting cache. I also remember that we recently removed count_private parameter and most likely a lot of people have not removed it from card URL. If the current strict check will be kept a lot of cards will be broken. I see two ways to solve it, first is reserving these query parameters, and second way is completely allowing to add extra parameters. I want to know which of them better in your opinion.

I would keep the current behavoir where we allow extra parameters, but let's see what @anuraghazra thinks 👍🏻.

After rethinking i came to the conclusion that there is no significant reasons to prohibit extra parameters. Are you up to approve this pull request for merging if i rework it this way?

@rickstaa I have update the code by covering changes with tests and allowing extra query string parameters. Check it please when you have time.