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 validation for DocumentKey #467

Merged
merged 22 commits into from
Feb 22, 2023

Conversation

easylogic
Copy link
Contributor

@easylogic easylogic commented Feb 14, 2023

What this PR does / why we need it:

The document key must be valid because it must be usable as the path segment of the url.

Which issue(s) this PR fixes:

Fixes #457

Special notes for your reviewer:

  • The Documentkey is used as the path segment of the url.
  • DocumentKey takes the form of a regular expression. (slugRegexString = ^[a-z0-9\-._~]+$)
  • The length of DocumentKey is limited to 4 ~ 120.

Does this PR introduce a user-facing change?:

* support validation for DocumentKey

Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

@CLAassistant
Copy link

CLAassistant commented Feb 14, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution and congratulations on your first PR. 🍻

I leave a few questions.

It seems that there are duplicate codes with validation.go, how should we solve it?

test/integration/array_test.go Outdated Show resolved Hide resolved
pkg/document/key/key_test.go Outdated Show resolved Hide resolved
@easylogic
Copy link
Contributor Author

@hackerwins

ci build test runner is different to local test runner.

There are no errro in local test runner.
But ci test runner has errors when test is build.

For example

last ci build has a test error.

It is a snapshot_test.go file.
But local test runner has no error.

Why it happened?

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #467 (e5dc4e3) into main (923b398) will decrease coverage by 0.15%.
The diff coverage is 60.18%.

@@            Coverage Diff             @@
##             main     #467      +/-   ##
==========================================
- Coverage   46.64%   46.49%   -0.15%     
==========================================
  Files          69       69              
  Lines        5765     5790      +25     
==========================================
+ Hits         2689     2692       +3     
- Misses       2771     2789      +18     
- Partials      305      309       +4     
Impacted Files Coverage Δ
server/rpc/yorkie_server.go 48.14% <0.00%> (-0.33%) ⬇️
internal/validation/validation_manager.go 51.21% <51.21%> (ø)
api/types/create_project_fields.go 87.50% <87.50%> (+87.50%) ⬆️
api/types/signup_fields.go 100.00% <100.00%> (ø)
api/types/updatable_project_fields.go 100.00% <100.00%> (ø)
pkg/document/key/key.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@easylogic
Copy link
Contributor Author

New validator

	err := validation.ValidateDynamically(k.String(), []any{
		"required",
		"slug",
		"min=4",
		"max=30",
		validation.CustomRule{
			Func: func(fl validator.FieldLevel) bool {
				return fl.Field().String()[0] == '$'
			},
		},
	})

	if err != nil {
		return ErrInvalidKey
	}

@hackerwins
Copy link
Member

We use Build Constraints for organizing the testing phase such as integration tests, benchmark tests, and so on. Additionally, Build Constraints can be used for specific machine architectures. As a result, tests could differ between local and CI.

You can find more information about Build Constraints in Go at https://pkg.go.dev/go/build#hdr-Build_Constraints

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

It would be nice to just clean up the minor code styles and then merge them.

Makefile Outdated Show resolved Hide resolved
internal/validation/validation_manager.go Outdated Show resolved Hide resolved
internal/validation/validation_manager.go Outdated Show resolved Hide resolved
pkg/document/key/key.go Outdated Show resolved Hide resolved
pkg/document/key/key_test.go Outdated Show resolved Hide resolved
test/helper/helper.go Outdated Show resolved Hide resolved
test/helper/helper.go Outdated Show resolved Hide resolved
test/helper/helper.go Outdated Show resolved Hide resolved
@hackerwins
Copy link
Member

hackerwins commented Feb 21, 2023

This will merged by #473

Sorry for the confusion. I was mistaken and thought it was just a matter of resolving conflicts and merging them. 🙏

@hackerwins hackerwins closed this Feb 21, 2023
@hackerwins hackerwins reopened this Feb 21, 2023
Makefile Outdated Show resolved Hide resolved
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. 👍
Now we have a validation module.

@hackerwins hackerwins merged commit ed541b9 into yorkie-team:main Feb 22, 2023
hackerwins added a commit that referenced this pull request Feb 23, 2023
hackerwins added a commit that referenced this pull request Feb 23, 2023
hackerwins added a commit that referenced this pull request Feb 23, 2023
@hackerwins
Copy link
Member

Users can express data information using keys. The 30-character key limit was too short, so I increased it to 120 characters.

https://firebase.google.com/docs/firestore/quotas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement validation for Document Key
4 participants