Validate `scale` param for `dbStats` and `collStats` correctly #2418

rumyantseva · 2023-04-12T15:19:31Z

Description

Closes #1346.

This PR is one of a few PRs for #1346. It is sent separately as it covers logic related to scale param validation and doesn't depend on the rest of the issue.

There is a minor refactoring: errLongExceeded error is split into errLongExceededNegative and errLongExceededPositive because, for the params like skip or scale, in case of negative overflow, we should still return a different error -- the value must be non-negative (for skip) or positive (for scale).

See also FerretDB/dance#407 for diff tests.

Readiness checklist

I added/updated unit tests.
I added/updated integration/compatibility tests.
I added/updated comments and checked rendering.
I made spot refactorings.
I updated user documentation.
I ran task all, and it passed.
I ensured that PR title is good enough for the changelog.
(for maintainers only) I set Reviewers (@FerretDB/core), Assignee, Labels, Project and project's Sprint fields.
I marked all done items in this checklist.

codecov · 2023-04-12T15:25:57Z

Codecov Report

Merging #2418 (3abcf2e) into main (f901668) will increase coverage by 0.08%.
The diff coverage is 77.31%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2418      +/-   ##
==========================================
+ Coverage   64.38%   64.46%   +0.08%     
==========================================
  Files         393      394       +1     
  Lines       19291    19349      +58     
==========================================
+ Hits        12421    12474      +53     
- Misses       5966     5972       +6     
+ Partials      904      903       -1

Impacted Files	Coverage Δ
internal/handlers/tigris/msg_collstats.go	`0.00% <0.00%> (ø)`
internal/handlers/tigris/msg_dbstats.go	`0.00% <0.00%> (ø)`
internal/handlers/pg/msg_update.go	`55.55% <37.50%> (+1.23%)`	⬆️
internal/handlers/pg/msg_dbstats.go	`69.38% <62.50%> (+1.99%)`	⬆️
internal/handlers/common/update.go	`87.29% <85.71%> (ø)`
internal/handlers/common/scale.go	`95.65% <95.65%> (ø)`
internal/handlers/common/aggregations/collstats.go	`96.07% <100.00%> (-0.08%)`	⬇️
internal/handlers/common/params.go	`95.98% <100.00%> (-0.02%)`	⬇️
internal/handlers/common/skip.go	`92.30% <100.00%> (+0.81%)`	⬆️
internal/handlers/pg/msg_collstats.go	`72.00% <100.00%> (+0.57%)`	⬆️

... and 1 file with indirect coverage changes

Flag	Coverage Δ
integration	`57.48% <77.31%> (+0.10%)`	⬆️
mongodb	`4.70% <0.00%> (-0.02%)`	⬇️
pg	`57.40% <77.31%> (+0.10%)`	⬆️
unit	`25.46% <1.68%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

…retDB into issue-1346-stats

chilagrow

I'm seeing that there will be other PRs, so the change I suggest can be done there too.

The failing one needs attention though 🤗

integration/commands_administration_compat_test.go

internal/handlers/common/scale.go

chilagrow

awesome work!

internal/handlers/pg/msg_dbstats.go

w84thesun

👍

AlekSi

LGTM for docs, but please add and update comments in the next PR

AlekSi · 2023-04-18T07:06:27Z

internal/handlers/common/params.go

@@ -121,7 +121,8 @@ var (
 	errNotBinaryMask         = fmt.Errorf("not a binary mask")
 	errUnexpectedLeftOpType  = fmt.Errorf("unexpected left operand type")
 	errUnexpectedRightOpType = fmt.Errorf("unexpected right operand type")
-	errLongExceeded          = fmt.Errorf("long exceeded")
+	errLongExceededPositive  = fmt.Errorf("long exceeded - positive value")
+	errLongExceededNegative  = fmt.Errorf("long exceeded - negative value")


I added/updated comments and checked rendering.

Before submitting a pull request, please make sure that:
[…]
2. Comments are added or updated for all new or changed code.
Please add missing comments for all (both exported and unexported)
new and changed top-level declarations (functions, types, etc).

Both points were ignored.

Updated possible error results are also not documented.

@AlekSi could you please leave more specific feedback? What exactly needs to be documented here?
Sometimes when I leave comments (e.g. for structure fields) you say that they are obvious and not needed. What needs to be documented here?

Fields are not top-level declarations. Global variables are.

Should I document what each error in this list means?
Or should I document each function/method where these errors could be returned?
Or both? Or something else?

…tDB#2418) Closes FerretDB#1346.

wip

c828017

AlekSi changed the title ~~wip~~ SchemaStats Apr 12, 2023

wip

a76b6d0

rumyantseva changed the title ~~SchemaStats~~ Validate scale param for dbStats and collStats correctly Apr 12, 2023

Merge branch 'main' into issue-1346-stats

e9db5a5

rumyantseva added code/chore Code maintenance improvements code/enhancement Some user-visible feature could work better and removed code/chore Code maintenance improvements labels Apr 12, 2023

rumyantseva self-assigned this Apr 12, 2023

rumyantseva added 10 commits April 12, 2023 19:20

wip

06df4cb

Merge branch 'issue-1346-stats' of https://github.com/rumyantseva/Fer…

092cb9d

…retDB into issue-1346-stats

wip

0939552

Merge branch 'main' into issue-1346-stats

17f3727

wip

a02f874

wip

065fc0f

linting things

83a80e9

wip

5ab050d

wip

324cb14

wip

124eb09

rumyantseva marked this pull request as ready for review April 12, 2023 22:10

rumyantseva requested a review from a team as a code owner April 12, 2023 22:10

rumyantseva requested review from AlekSi, w84thesun and chilagrow April 12, 2023 22:10

rumyantseva enabled auto-merge (squash) April 12, 2023 22:10

rumyantseva requested review from a team and noisersup April 12, 2023 22:10

Merge branch 'main' into issue-1346-stats

eabaaa0

chilagrow reviewed Apr 13, 2023

View reviewed changes

integration/commands_administration_compat_test.go Outdated Show resolved Hide resolved

internal/handlers/common/scale.go Show resolved Hide resolved

internal/handlers/common/scale.go Show resolved Hide resolved

Merge branch 'main' into issue-1346-stats

d70a4e6

chilagrow previously approved these changes Apr 17, 2023

View reviewed changes

AlekSi added this to the v1.1.0 milestone Apr 17, 2023

AlekSi and others added 3 commits April 17, 2023 10:16

Merge branch 'main' into issue-1346-stats

318cd6e

Merge branch 'main' into issue-1346-stats

a5c8c9d

wip

56e1dc5

rumyantseva dismissed stale reviews from w84thesun, noisersup, and chilagrow via 56e1dc5 April 17, 2023 15:13

rumyantseva added 2 commits April 17, 2023 17:15

wip

8a7adda

wip

f0b5a46

rumyantseva requested review from w84thesun, noisersup and chilagrow April 17, 2023 16:02

rumyantseva added 3 commits April 17, 2023 18:28

wip

c302ef8

wip

ff5708a

wip

8c5e55c

rumyantseva commented Apr 17, 2023

View reviewed changes

internal/handlers/pg/msg_dbstats.go Show resolved Hide resolved

Merge branch 'main' into issue-1346-stats

0ac1cd5

w84thesun approved these changes Apr 17, 2023

View reviewed changes

noisersup approved these changes Apr 17, 2023

View reviewed changes

chilagrow approved these changes Apr 18, 2023

View reviewed changes

AlekSi approved these changes Apr 18, 2023

View reviewed changes

Merge branch 'main' into issue-1346-stats

3abcf2e

rumyantseva merged commit 548114f into FerretDB:main Apr 18, 2023

rumyantseva deleted the issue-1346-stats branch April 18, 2023 07:44

fenogentov pushed a commit to fenogentov/FerretDB that referenced this pull request Apr 20, 2023

Validate scale param for dbStats and collStats correctly (Ferre…

b4dab52

…tDB#2418) Closes FerretDB#1346.

fenogentov pushed a commit to fenogentov/FerretDB that referenced this pull request Apr 20, 2023

Validate scale param for dbStats and collStats correctly (Ferre…

cb2dabe

…tDB#2418) Closes FerretDB#1346.

fenogentov pushed a commit to fenogentov/FerretDB that referenced this pull request Apr 20, 2023

Validate scale param for dbStats and collStats correctly (Ferre…

94f931c

…tDB#2418) Closes FerretDB#1346.

fenogentov pushed a commit to fenogentov/FerretDB that referenced this pull request Apr 20, 2023

Validate scale param for dbStats and collStats correctly (Ferre…

ce60768

…tDB#2418) Closes FerretDB#1346.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate `scale` param for `dbStats` and `collStats` correctly #2418

Validate `scale` param for `dbStats` and `collStats` correctly #2418

rumyantseva commented Apr 12, 2023 •

edited

Loading

codecov bot commented Apr 12, 2023 •

edited

Loading

chilagrow left a comment

chilagrow left a comment

w84thesun left a comment

AlekSi left a comment

AlekSi Apr 18, 2023

rumyantseva Apr 18, 2023

AlekSi Apr 18, 2023

rumyantseva Apr 18, 2023

Validate scale param for dbStats and collStats correctly #2418

Validate scale param for dbStats and collStats correctly #2418

Conversation

rumyantseva commented Apr 12, 2023 • edited Loading

Description

Readiness checklist

codecov bot commented Apr 12, 2023 • edited Loading

Codecov Report

chilagrow left a comment

Choose a reason for hiding this comment

chilagrow left a comment

Choose a reason for hiding this comment

w84thesun left a comment

Choose a reason for hiding this comment

AlekSi left a comment

Choose a reason for hiding this comment

AlekSi Apr 18, 2023

Choose a reason for hiding this comment

rumyantseva Apr 18, 2023

Choose a reason for hiding this comment

AlekSi Apr 18, 2023

Choose a reason for hiding this comment

rumyantseva Apr 18, 2023

Choose a reason for hiding this comment

Validate `scale` param for `dbStats` and `collStats` correctly #2418

Validate `scale` param for `dbStats` and `collStats` correctly #2418

rumyantseva commented Apr 12, 2023 •

edited

Loading

codecov bot commented Apr 12, 2023 •

edited

Loading