Fix invalid validation for `_id` field #3523

slavabobik · 2023-10-07T12:09:54Z

Description

Closes #2804.

Readiness checklist

I added/updated unit tests (and they pass).
I added/updated integration/compatibility tests (and they pass).
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), Milestone (Next), Labels, Project and project's Sprint fields.
I marked all done items in this checklist.

codecov · 2023-10-07T12:13:52Z

Codecov Report

Merging #3523 (ecc2f20) into main (29f7fe6) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3523      +/-   ##
==========================================
- Coverage   61.12%   61.05%   -0.07%     
==========================================
  Files         426      426              
  Lines       27185    27187       +2     
==========================================
- Hits        16616    16600      -16     
- Misses       9573     9594      +21     
+ Partials      996      993       -3

Files	Coverage Δ
internal/types/document_validation.go	`100.00% <100.00%> (ø)`

... and 18 files with indirect coverage changes

Flag	Coverage Δ
filter-true	`57.58% <100.00%> (-0.07%)`	⬇️
hana-1	`?`
integration	`57.58% <100.00%> (-0.07%)`	⬇️
mongodb-1	`4.51% <0.00%> (-0.01%)`	⬇️
postgresql-1	`41.43% <100.00%> (+<0.01%)`	⬆️
postgresql-2	`38.87% <66.66%> (+<0.01%)`	⬆️
postgresql-3	`41.25% <66.66%> (+<0.01%)`	⬆️
sort-false	`57.58% <100.00%> (-0.07%)`	⬇️
sqlite-1	`41.86% <100.00%> (?)`
sqlite-2	`39.10% <66.66%> (-0.03%)`	⬇️
sqlite-3	`?`
unit	`22.37% <100.00%> (+0.03%)`	⬆️

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

slavabobik · 2023-10-07T12:25:40Z

I'm not so sure that this is a better solution, I have a couple of thoughts:

Instead of creating every time new boolean variable topLevel inside validateData(..) function, may be better add two helper function like

func (d *Document) validateTopLevelData() error { return d.validateData(true) }
func (d *Document) validateInnerData() error { return d.validateData(false) }

and inside d.validateData(...) func we could use it like:

...
case *Document:
    err := value.validateInnerData()
...

But this approach add complexity and not clarity

May be the better solution(instead of passing stave over recursion functions), is add in the Document struct boolean field root, but I'm not sure how easy to populate this field.

@AlekSi what do you think?

AlekSi

I think the current approach is simple and good, it just needs a small clean-up. There is no need to shadow and redefine that variable

internal/types/document_validation.go

slavabobik · 2023-10-08T15:04:52Z

Agree, my first attempt was like you suggested 😄

rumyantseva · 2023-10-09T09:29:46Z

Thanks for contributing! It would also be nice to add integration tests for this case, so we ensure that the behavior is compatible with MongoDB.

I think shareddata.DocumentsDocuments might be a good candidate. For example, we can add three more docs there - for array, document and regex. Maybe something like this:

var DocumentsDocuments = &Values[primitive.ObjectID]{
	name: "DocumentsDocuments",
	data: map[primitive.ObjectID]any{
		{0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01}: bson.D{{"foo", int32(42)}},
		{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}: bson.D{{"bar", bson.D{}}},
		{0x02, 0x02, 0x02, 0x02, 0x02, 0x02, 0x02, 0x02, 0x02, 0x02, 0x02, 0x02}: bson.D{{"_id", bson.A{int32(42), int32(42)}}},
		{0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x03}: bson.D{{"_id", bson.D{{"foo", "bar"}}}},
		{0x04, 0x04, 0x04, 0x04, 0x04, 0x04, 0x04, 0x04, 0x04, 0x04, 0x04, 0x04}: bson.D{{"_id", primitive.Regex{Pattern: "foo", Options: "i"}}},
	},
}

As these documents are inserted when the integration tests are run, there will be a check that the insertion was successful.

Alternatively, please feel free to suggest some other tests.

rumyantseva

Also, please feel free to ping me if you need any help.

AlekSi

#3523 (comment)

slavabobik · 2023-10-11T17:30:42Z

Hi @rumyantseva! thank you for your examples, I slightly adjust the last example about regex field.
Can you please help me with Go / postgresql 1/{1,2,3} / Run (pull_request) tests? I can't get it green, and I'm not sure why?
also when I run task all locally, these tests are not green
Now they are all green

slavabobik · 2023-10-11T18:19:57Z

@rumyantseva can you please help me investigate one test TestDefaults? My local task all run fails on this test, but I'll try my branch and main, result is the same, there is short log of this test:

 FAIL: TestDefaults (0.01s)
    logger.go:130: 2023-10-11T21:15:57.825+0300	DEBUG	pool/pool.go:201	Database created.	{"name": "TestDefaults", "uri": "file:TestDefaults.sqlite?_pragma=auto_vacuum%28none%29&_pragma=busy_timeout%2810000%29&_pragma=journal_mode%28wal%29"}
    logger.go:130: 2023-10-11T21:15:57.825+0300	DEBUG	TestDefaults	fsql/db.go:77	>>> PRAGMA compile_options	{"args": null}
    logger.go:130: 2023-10-11T21:15:57.825+0300	DEBUG	TestDefaults	fsql/db.go:82	<<< PRAGMA compile_options	{"args": null, "time": "192.791µs"}
.....
    logger.go:130: 2023-10-11T21:15:57.827+0300	DEBUG	TestDefaults	fsql/db.go:94	>>> PRAGMA encoding	{"args": null}
    logger.go:130: 2023-10-11T21:15:57.827+0300	DEBUG	TestDefaults	fsql/db.go:94	>>> PRAGMA journal_mode	{"args": null}
    logger.go:130: 2023-10-11T21:15:57.827+0300	DEBUG	TestDefaults	fsql/db.go:94	>>> PRAGMA busy_timeout	{"args": null}
    logger.go:130: 2023-10-11T21:15:57.827+0300	DEBUG	TestDefaults	fsql/db.go:94	>>> PRAGMA auto_vacuum	{"args": null}
    logger.go:130: 2023-10-11T21:15:57.827+0300	DEBUG	TestDefaults	fsql/db.go:94	>>> SELECT sqlite_source_id()	{"args": null}
    logger.go:130: 2023-10-11T21:15:57.827+0300	DEBUG	TestDefaults	fsql/db.go:94	>>> SELECT sqlite_version()	{"args": null}
    logger.go:130: 2023-10-11T21:15:57.827+0300	DEBUG	TestDefaults	fsql/db.go:99	<<< PRAGMA encoding	{"args": null, "time": "391.791µs"}
    logger.go:130: 2023-10-11T21:15:57.830+0300	DEBUG	TestDefaults	fsql/db.go:99	<<< PRAGMA busy_timeout	{"args": null, "time": "3.398584ms", "error": "database is locked (261)"}
    logger.go:130: 2023-10-11T21:15:57.830+0300	DEBUG	TestDefaults	fsql/db.go:99	<<< PRAGMA auto_vacuum	{"args": null, "time": "3.260208ms", "error": "database is locked (261)"}
    logger.go:130: 2023-10-11T21:15:57.830+0300	DEBUG	TestDefaults	fsql/db.go:99	<<< SELECT sqlite_version()	{"args": null, "time": "3.52925ms", "error": "database is locked (261)"}
    --- FAIL: TestDefaults/PRAGMA_busy_timeout (0.00s)
        pool_test.go:203:
            	Error Trace:	/Users/slavabobik/Code/FerretDB/internal/backends/sqlite/metadata/pool/pool_test.go:203
            	Error:      	Received unexpected error:
            	            	database is locked (261)
            	Test:       	TestDefaults/PRAGMA_busy_timeout
    --- FAIL: TestDefaults/PRAGMA_auto_vacuum (0.00s)
        pool_test.go:203:
            	Error Trace:	/Users/slavabobik/Code/FerretDB/internal/backends/sqlite/metadata/pool/pool_test.go:203
            	Error:      	Received unexpected error:
            	            	database is locked (261)
            	Test:       	TestDefaults/PRAGMA_auto_vacuum
    logger.go:130: 2023-10-11T21:15:57.830+0300	DEBUG	TestDefaults	fsql/db.go:99	<<< PRAGMA journal_mode	{"args": null, "time": "3.511708ms", "error": "database is locked (261)"}
    --- FAIL: TestDefaults/SELECT_sqlite_version() (0.00s)
        pool_test.go:203:
            	Error Trace:	/Users/slavabobik/Code/FerretDB/internal/backends/sqlite/metadata/pool/pool_test.go:203
            	Error:      	Received unexpected error:
            	            	database is locked (261)
            	Test:       	TestDefaults/SELECT_sqlite_version()
    --- FAIL: TestDefaults/PRAGMA_journal_mode (0.00s)
        pool_test.go:203:
            	Error Trace:	/Users/slavabobik/Code/FerretDB/internal/backends/sqlite/metadata/pool/pool_test.go:203
            	Error:      	Received unexpected error:
            	            	database is locked (261)
            	Test:       	TestDefaults/PRAGMA_journal_mode
    logger.go:130: 2023-10-11T21:15:57.830+0300	DEBUG	TestDefaults	fsql/db.go:99	<<< SELECT sqlite_source_id()	{"args": null, "time": "3.584792ms"}
    logger.go:130: 2023-10-11T21:15:57.831+0300	DEBUG	pool/pool.go:236	Database dropped.	{"name": "TestDefaults"}
FAIL
exit status 1
FAIL	github.com/FerretDB/FerretDB/internal/backends/sqlite/metadata/pool	1.097s

AlekSi · 2023-10-11T18:21:57Z

It is not you – it's us :) Just ignore it. We plan to fix that soon

rumyantseva

Thank you for contributing!

The changes look good to me (I manually tested the cases with valid/invalid top-level _id fields as we don't have integration tests for it).

rumyantseva · 2023-10-11T20:08:40Z

internal/types/document_validation.go

@@ -115,7 +120,7 @@ func (d *Document) ValidateData() error {
 				return err
 			}
 		case *Array:
-			if key == "_id" {
+			if isTopLevel && key == "_id" {


@slavabobik codecov still marks some lines uncovered, it's because we don't have tests where we try to insert documents with top-level _id set as array or regex. A good candidate for such tests could be TestInsertCommandErrors, we can add examples where top-level _id field is filled with those invalid types and we know which error we expect. Personally, I would add such tests, but in this PR I leave it up to you to do it (if you have more time and wish, please feel free to do it, otherwise we can do it separately).

What else we don't have is a test case where top-level _id is set as a document (a valid case, but probably rate in real life), but it's rather a separate story :)

@rumyantseva I can add more tests in separate PR :) Do I need a separate task or just create a PR with tests?

Let‘s have a separate PR only. A separate task is not needed. Thank you!

integration/shareddata/composites.go

added applying different rules for _id valudation

530e431

slavabobik requested a review from a team as a code owner October 7, 2023 12:09

slavabobik requested review from AlekSi and rumyantseva October 7, 2023 12:09

mergify bot assigned slavabobik Oct 7, 2023

slavabobik mentioned this pull request Oct 7, 2023

Invalid validation for _id field #2804

Closed

AlekSi reviewed Oct 7, 2023

View reviewed changes

internal/types/document_validation.go Outdated Show resolved Hide resolved

internal/types/document_validation.go Outdated Show resolved Hide resolved

internal/types/document_validation.go Outdated Show resolved Hide resolved

added small clean-up

1eb2bbf

slavabobik requested a review from AlekSi October 8, 2023 14:57

rumyantseva added this to the Next milestone Oct 9, 2023

rumyantseva added the code/bug Some user-visible feature works incorrectly label Oct 9, 2023

rumyantseva changed the title ~~Fix Invalid validation for _id field~~ Fix Invalid validation for _id field Oct 9, 2023

AlekSi requested review from AlekSi and removed request for AlekSi October 9, 2023 09:24

AlekSi assigned rumyantseva Oct 9, 2023

AlekSi changed the title ~~Fix Invalid validation for _id field~~ Fix invalid validation for _id field Oct 9, 2023

rumyantseva reviewed Oct 10, 2023

View reviewed changes

AlekSi reviewed Oct 11, 2023

View reviewed changes

slavabobik added 2 commits October 11, 2023 19:53

added inegration test cases

2698cb4

updated regex document for integration tests

ef06d05

slavabobik requested review from rumyantseva and AlekSi October 11, 2023 17:33

Merge branch 'main' into fix-document_id-validation

255469d

AlekSi enabled auto-merge (squash) October 11, 2023 18:14

rumyantseva approved these changes Oct 11, 2023

View reviewed changes

AlekSi reviewed Oct 12, 2023

View reviewed changes

integration/shareddata/composites.go Show resolved Hide resolved

AlekSi approved these changes Oct 12, 2023

View reviewed changes

mergify bot and others added 2 commits October 12, 2023 08:24

Merge branch 'main' into fix-document_id-validation

3cc9fa1

Merge branch 'main' into fix-document_id-validation

ecc2f20

AlekSi merged commit 4801d37 into FerretDB:main Oct 12, 2023
26 checks passed

slavabobik mentioned this pull request Oct 15, 2023

Add tests for insertion of documents with invalid _id fields #3579

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalid validation for `_id` field #3523

Fix invalid validation for `_id` field #3523

slavabobik commented Oct 7, 2023 •

edited

Loading

codecov bot commented Oct 7, 2023 •

edited

Loading

slavabobik commented Oct 7, 2023 •

edited

Loading

AlekSi left a comment

slavabobik commented Oct 8, 2023

rumyantseva commented Oct 9, 2023

rumyantseva left a comment

AlekSi left a comment

slavabobik commented Oct 11, 2023 •

edited

Loading

slavabobik commented Oct 11, 2023

AlekSi commented Oct 11, 2023

rumyantseva left a comment

rumyantseva Oct 11, 2023

slavabobik Oct 12, 2023

rumyantseva Oct 12, 2023

Fix invalid validation for _id field #3523

Fix invalid validation for _id field #3523

Conversation

slavabobik commented Oct 7, 2023 • edited Loading

Description

Readiness checklist

codecov bot commented Oct 7, 2023 • edited Loading

Codecov Report

slavabobik commented Oct 7, 2023 • edited Loading

AlekSi left a comment

Choose a reason for hiding this comment

slavabobik commented Oct 8, 2023

rumyantseva commented Oct 9, 2023

rumyantseva left a comment

Choose a reason for hiding this comment

AlekSi left a comment

Choose a reason for hiding this comment

slavabobik commented Oct 11, 2023 • edited Loading

slavabobik commented Oct 11, 2023

AlekSi commented Oct 11, 2023

rumyantseva left a comment

Choose a reason for hiding this comment

rumyantseva Oct 11, 2023

Choose a reason for hiding this comment

slavabobik Oct 12, 2023

Choose a reason for hiding this comment

rumyantseva Oct 12, 2023

Choose a reason for hiding this comment

Fix invalid validation for `_id` field #3523

Fix invalid validation for `_id` field #3523

slavabobik commented Oct 7, 2023 •

edited

Loading

codecov bot commented Oct 7, 2023 •

edited

Loading

slavabobik commented Oct 7, 2023 •

edited

Loading

slavabobik commented Oct 11, 2023 •

edited

Loading