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

Support all valid collection names #778

Merged

Conversation

w84thesun
Copy link
Contributor

@w84thesun w84thesun commented Jun 23, 2022

Closes #750.

Checklist.

  • Tests are added for new functionality or fixed bugs.
  • task all passes.

See CONTRIBUTING.md for more details.

Copy link
Member

@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.

I had a look. My biggest concern is race conditions.
Should we discuss how are we going to avoid them?

internal/handlers/pg/pgdb/pool.go Show resolved Hide resolved
internal/handlers/pg/pgdb/pool.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/pool.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/pool.go Outdated Show resolved Hide resolved
@w84thesun w84thesun self-assigned this Jun 23, 2022
@w84thesun w84thesun added code/feature Some user-visible feature is not implemented yet code/enhancement Some user-visible feature could work better and removed code/feature Some user-visible feature is not implemented yet labels Jun 23, 2022
@w84thesun w84thesun added this to the v0.3.1 milestone Jun 23, 2022
@AlekSi AlekSi removed this from the v0.4.0 milestone Jun 23, 2022
@codecov
Copy link

codecov bot commented Jun 24, 2022

Codecov Report

Merging #778 (9214a0e) into main (a8a0975) will decrease coverage by 0.66%.
The diff coverage is 48.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #778      +/-   ##
==========================================
- Coverage   59.58%   58.92%   -0.66%     
==========================================
  Files         216      217       +1     
  Lines        9475     9817     +342     
==========================================
+ Hits         5646     5785     +139     
- Misses       3177     3323     +146     
- Partials      652      709      +57     
Impacted Files Coverage Δ
internal/util/testutil/pg.go 40.74% <0.00%> (-0.73%) ⬇️
internal/handlers/pg/msg_insert.go 40.00% <14.28%> (-0.91%) ⬇️
internal/handlers/pg/pgdb/settings.go 37.20% <37.20%> (ø)
internal/handlers/pg/msg_create.go 67.24% <50.00%> (ø)
internal/handlers/pg/pgdb/pool.go 57.19% <51.82%> (-8.68%) ⬇️
internal/handlers/pg/msg_delete.go 44.08% <85.71%> (-3.92%) ⬇️
internal/handlers/pg/msg_update.go 53.19% <85.71%> (-0.33%) ⬇️
internal/handlers/pg/fetch.go 66.66% <100.00%> (+0.62%) ⬆️
internal/handlers/pg/msg_drop.go 61.76% <100.00%> (+1.15%) ⬆️
internal/handlers/pg/msg_dropdatabase.go 62.06% <100.00%> (ø)
... and 3 more
Flag Coverage Δ
FerretDB 61.15% <46.88%> (-0.98%) ⬇️
MongoDB 7.10% <0.00%> (-0.36%) ⬇️
integration 61.24% <46.88%> (-0.98%) ⬇️
unit 27.61% <29.17%> (+0.04%) ⬆️

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

seeforschauer
seeforschauer previously approved these changes Jun 30, 2022
Copy link
Contributor

@seeforschauer seeforschauer left a comment

Choose a reason for hiding this comment

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

@w84thesun sorry, tests

@AlekSi
Copy link
Member

AlekSi commented Jun 30, 2022

See #815

@AlekSi AlekSi disabled auto-merge June 30, 2022 20:48
@AlekSi AlekSi enabled auto-merge (squash) June 30, 2022 20:48
Copy link
Member

@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.

It looks to me that the last place where tableName and collectionName are mixed is in the tests. But there will be tons of tests to refactor! So, I think personally I'm ready to approve what we have now. Maybe we can refactor names in tests later?

@w84thesun you rock! That was a huge refactoring!

}, {
name: "CreateCollection",
f: func() error {
return pool.CreateCollection(ctx, schemaName, tableName)
Copy link
Member

Choose a reason for hiding this comment

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

Is this tableName should we a collectionName now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I suggest working on those tests in a separate PR. We might want to add more tests for the Pool.

Copy link
Member

Choose a reason for hiding this comment

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

Ha, I did not read it before review. Good to see us on the same page

Copy link
Member

@AlekSi AlekSi left a comment

Choose a reason for hiding this comment

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

LGTM to unblock Elena

@@ -129,13 +128,13 @@ func Table(ctx context.Context, tb testing.TB, pool *pgdb.Pool, db string) strin
table := TableName(tb)
Copy link
Member

Choose a reason for hiding this comment

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

@rumyantseva Please clean-up those helpers in your PR. For example, it is not clean if this helper return table name (with a hash) or collection name. At very least, documentation should be updated. Maybe name too

Copy link
Member

Choose a reason for hiding this comment

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

👌

@AlekSi AlekSi disabled auto-merge July 1, 2022 11:44
@AlekSi AlekSi merged commit a545af6 into FerretDB:main Jul 1, 2022
ribaraka pushed a commit to ribaraka/FerretDB that referenced this pull request Jul 1, 2022
@w84thesun w84thesun deleted the issue-750-support-valid-collection-names branch July 4, 2022 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/enhancement Some user-visible feature could work better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support all valid collection names
4 participants