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

StatObject validates names twice #1583

Closed
bboreham opened this issue Nov 15, 2021 · 5 comments · Fixed by #1584
Closed

StatObject validates names twice #1583

bboreham opened this issue Nov 15, 2021 · 5 comments · Fixed by #1584
Labels

Comments

@bboreham
Copy link

Either statObject should not be doing the validation, or StatObject is redundant, because the first 7 lines of each function are identical.

minio-go/api-stat.go

Lines 60 to 80 in daea25a

// StatObject verifies if object exists and you have permission to access.
func (c *Client) StatObject(ctx context.Context, bucketName, objectName string, opts StatObjectOptions) (ObjectInfo, error) {
// Input validation.
if err := s3utils.CheckValidBucketName(bucketName); err != nil {
return ObjectInfo{}, err
}
if err := s3utils.CheckValidObjectName(objectName); err != nil {
return ObjectInfo{}, err
}
return c.statObject(ctx, bucketName, objectName, opts)
}
// Lower level API for statObject supporting pre-conditions and range headers.
func (c *Client) statObject(ctx context.Context, bucketName, objectName string, opts StatObjectOptions) (ObjectInfo, error) {
// Input validation.
if err := s3utils.CheckValidBucketName(bucketName); err != nil {
return ObjectInfo{}, err
}
if err := s3utils.CheckValidObjectName(objectName); err != nil {
return ObjectInfo{}, err
}

klauspost added a commit to klauspost/minio-go that referenced this issue Nov 15, 2021
Clean up duplicate internal function.

Fixes minio#1583
@harshavardhana
Copy link
Member

Either statObject should not be doing the validation, or StatObject is redundant, because the first 7 lines of each function are identical.

what is the problem if it does double validation? @bboreham - feel free to clean it up if you wish.

It is simply duplicated due to layers of re-use and historical.

@bboreham
Copy link
Author

Problems include run-time cost and unnecessary code for the reader to understand.

@harshavardhana
Copy link
Member

Problems include run-time cost and unnecessary code for the reader to understand.

runtime cost? can you show me?

harshavardhana pushed a commit that referenced this issue Nov 15, 2021
Clean up duplicate internal functions.

fixes #1583
@bboreham
Copy link
Author

I don't know why you're being so aggressive here.

if !validBucketNameStrict.MatchString(bucketName) {
err = errors.New("Bucket name contains invalid characters")
}
return err
}
if !validBucketName.MatchString(bucketName) {

@harshavardhana
Copy link
Member

I don't know why you're being so aggressive here.

Not mean to be aggressive, our wish is to get more information into this issue.

Since you said runtime cost - can you share how you measured it? we would like to avoid costs that are greater than the HTTP costs so it is important that we get enough information.

However, the de-duplication of the code is fixed in the master branch.

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

Successfully merging a pull request may close this issue.

2 participants