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.

Sign up for GitHub

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

Jump to bottom

Allow database name contain uppercase characters #2504

Merged
merged 15 commits into from
Apr 26, 2023
Merged

Allow database name contain uppercase characters #2504

merged 15 commits into from
Apr 26, 2023

Conversation

syasyayas
Copy link
Contributor

@syasyayas syasyayas commented Apr 22, 2023 •
edited by Fashander
Loading

Description

Closes #2451.

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), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@syasyayas syasyayas requested a review from a team as a code owner April 22, 2023 18:09
@syasyayas syasyayas requested review from AlekSi and w84thesun April 22, 2023 18:09
@syasyayas syasyayas requested a review from ptrfarkas as a code owner April 22, 2023 18:11
@syasyayas
Copy link
Contributor Author

syasyayas commented Apr 22, 2023 •
edited
Loading

Checking tests here because my computer dies on task test :)

@codecov
Copy link

codecov bot commented Apr 24, 2023 •
edited
Loading

Codecov Report

Merging #2504 (a260d10) into main (11eb71d) will increase coverage by 0.02%.
The diff coverage is 33.33%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2504      +/-   ##
==========================================
+ Coverage   26.75%   26.78%   +0.02%     
==========================================
  Files         403      403              
  Lines       19976    19976              
==========================================
+ Hits         5345     5350       +5     
+ Misses      14065    14061       -4     
+ Partials      566      565       -1
Impacted Files Coverage Δ
internal/handlers/pg/pgdb/databases.go 22.34% <ø> (ø)
internal/handlers/pg/pgdb/stats.go 0.00% <0.00%> (ø)
internal/util/testutil/db.go 87.30% <100.00%> (ø)

... and 2 files with indirect coverage changes

Flag Coverage Δ
integration 5.06% <33.33%> (ø)
mongodb 5.06% <33.33%> (ø)
unit 25.38% <33.33%> (+0.02%) ⬆️

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

@w84thesun w84thesun enabled auto-merge (squash) April 24, 2023 14:55
@w84thesun
Copy link
Contributor

w84thesun commented Apr 24, 2023 •
edited
Loading

Hey!
There were a couple of places where we didn't sanitize the DB name as we should have. That's why the test are failing.
It's in internal/handlers/pg/pgdb/stats.go.
First one the line 90:

	sql = `
		SELECT 
		    SUM(pg_total_relation_size(quote_ident(schemaname) || '.' || quote_ident(tablename))) 
		FROM pg_tables 
		WHERE schemaname = $1`
	args := []any{pgx.Identifier{db}.Sanitize()}

The second one is on line 165:

		WHERE oid = '%s'::regclass`,
		pgx.Identifier{db, metadata.table}.Sanitize(),

Could you please fix those 2 places?

@syasyayas
Copy link
Contributor Author

@w84thesun, yeah sure! But what about other places like:

FerretDB/internal/handlers/pg/pgdb/stats.go

Line 178 in e44c4d3

args := []any{db, metadata.table}

FerretDB/internal/handlers/pg/pgdb/stats.go

Line 122 in e44c4d3

args = []any{db, reservedPrefix + "%"}

auto-merge was automatically disabled April 24, 2023 22:05

Head branch was pushed to by a user without write access

@w84thesun
Copy link
Contributor

@w84thesun, yeah sure! But what about other places like:

Thanks for noticing and handling those places!

w84thesun
w84thesun previously approved these changes Apr 25, 2023
View reviewed changes
Copy link
Contributor

@w84thesun w84thesun left a comment

Choose a reason for hiding this comment

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

LGTM!

@w84thesun w84thesun requested review from a team, rumyantseva, chilagrow and noisersup April 25, 2023 07:59
@w84thesun w84thesun enabled auto-merge (squash) April 25, 2023 08:01
chilagrow
chilagrow previously approved these changes Apr 25, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Great work!

noisersup
noisersup previously approved these changes Apr 25, 2023
View reviewed changes
Copy link
Member

@noisersup noisersup left a comment

Choose a reason for hiding this comment

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

Looks great!

@syasyayas
Copy link
Contributor Author

I am not sure why pg tests fails...

logger.go:130: 2023-04-25T08:37:56.849Z	ERROR	pgdb	tracelog/tracelog.go:335	Query	{"err": "ERROR: schema \"TestCommandsAdministrationCollStatsWithScale\" does not exist (SQLSTATE 3F000)", "time": "298.304µs", "pid": 332, "sql": "DROP SCHEMA \"TestCommandsAdministrationCollStatsWithScale\" CASCADE", "args": []}

I am kinda confused looking at this logs as both CreateDatabaseIfNotExists and DropDatabase in https://github.com/FerretDB/FerretDB/blob/main/internal/handlers/pg/pgdb/databases.go both sanitize db names.

@w84thesun
Copy link
Contributor

I am not sure why pg tests fails...

I'll take a look at it today.

@w84thesun w84thesun dismissed stale reviews from noisersup, chilagrow, and themself via a5d1b8e April 26, 2023 15:56
w84thesun
w84thesun approved these changes Apr 26, 2023
View reviewed changes
Copy link
Contributor

@w84thesun w84thesun left a comment

Choose a reason for hiding this comment

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

LGTM!

rumyantseva
rumyantseva approved these changes Apr 26, 2023
View reviewed changes
Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

LGTM

@w84thesun w84thesun merged commit 4088762 into FerretDB:main Apr 26, 2023
@syasyayas
Copy link
Contributor Author

Thanks for help!

@ptrfarkas
Copy link
Member

@syasyayas we thank you for your valuable contribution to FerretDB!

@AlekSi AlekSi added the code/enhancement Some user-visible feature could work better label May 8, 2023
@AlekSi AlekSi added this to the v1.1.0 milestone May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptrfarkas ptrfarkas ptrfarkas approved these changes

@rumyantseva rumyantseva rumyantseva approved these changes

@w84thesun w84thesun w84thesun approved these changes

@AlekSi AlekSi Awaiting requested review from AlekSi AlekSi is a code owner automatically assigned from FerretDB/leads

@chilagrow chilagrow Awaiting requested review from chilagrow

@noisersup noisersup Awaiting requested review from noisersup

Assignees

@syasyayas syasyayas

Labels
code/enhancement Some user-visible feature could work better
Projects
Current Engineering
Archived in project
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.

Allow uppercase Latin letters in database names
7 participants
@syasyayas @w84thesun @ptrfarkas @rumyantseva @chilagrow @noisersup @AlekSi