You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/08/19 11:49:24 UTC
[GitHub] [beam] vchunikhin opened a new pull request, #22792: [Playground] [Backend] added SDK validation to save a code snippet
vchunikhin opened a new pull request, #22792:
URL: https://github.com/apache/beam/pull/22792
fixes #22789
@olehborysevych
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on pull request #22792: [Playground] [Backend] added SDK validation to save a code snippet
Posted by GitBox <gi...@apache.org>.
pabloem commented on PR #22792:
URL: https://github.com/apache/beam/pull/22792#issuecomment-1230657522
lgtm otherwise. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] olehborysevych commented on a diff in pull request #22792: [Playground] [Backend] added SDK validation to save a code snippet
Posted by GitBox <gi...@apache.org>.
olehborysevych commented on code in PR #22792:
URL: https://github.com/apache/beam/pull/22792#discussion_r950122219
##########
playground/backend/cmd/server/controller_test.go:
##########
@@ -813,7 +815,7 @@ func TestPlaygroundController_SaveSnippet(t *testing.T) {
},
},
wantErr: false,
- wantId: "xHce_LOg7Zm",
+ wantId: "l7OFah5mLHU",
Review Comment:
@vchunikhin what changed here so that id is changed?
##########
playground/backend/internal/utils/file_utils.go:
##########
@@ -35,53 +37,105 @@ const (
goExt = ".go"
pythonExt = ".py"
scioExt = ".scala"
+
+ mismatchExtAndSDKErrMsg = "file extension doesn't match to the specified SDK"
)
// GetFileName returns the valid file name.
-func GetFileName(name string, sdk pb.Sdk) string {
+func GetFileName(name, content string, sdk pb.Sdk) (string, error) {
if name == "" {
logger.Warn("The name of the file is empty. Will be used default value")
switch sdk {
case pb.Sdk_SDK_JAVA:
- return defaultJavaFileName
+ return defaultJavaFileName, nil
case pb.Sdk_SDK_GO:
- return defaultGoFileName
+ return defaultGoFileName, nil
case pb.Sdk_SDK_PYTHON:
- return defaultPythonFileName
+ return defaultPythonFileName, nil
case pb.Sdk_SDK_SCIO:
- return defaultScioFileName
+ return defaultScioFileName, nil
}
}
- return getCorrectFileName(name, sdk)
+ return getCorrectFileName(name, content, sdk)
}
// getCorrectFileName returns the correct file name.
-func getCorrectFileName(name string, sdk pb.Sdk) string {
- ext := filepath.Ext(name)
+func getCorrectFileName(name, content string, sdk pb.Sdk) (string, error) {
+ ext := getExtOrExtBasedOnContent(filepath.Ext(name), content)
+ if !isValidFileExtensionAndSDK(ext, sdk) {
+ return "", errors.New(mismatchExtAndSDKErrMsg)
+ }
switch sdk {
case pb.Sdk_SDK_JAVA:
- return getCorrectNameOrDefault(ext, javaExt, defaultJavaFileName, name)
+ return getCorrectNameOrDefault(ext, javaExt, defaultJavaFileName, name), nil
case pb.Sdk_SDK_GO:
- return getCorrectNameOrDefault(ext, goExt, defaultGoFileName, name)
+ return getCorrectNameOrDefault(ext, goExt, defaultGoFileName, name), nil
case pb.Sdk_SDK_PYTHON:
- return getCorrectNameOrDefault(ext, pythonExt, defaultPythonFileName, name)
+ return getCorrectNameOrDefault(ext, pythonExt, defaultPythonFileName, name), nil
case pb.Sdk_SDK_SCIO:
- return getCorrectNameOrDefault(ext, scioExt, defaultScioFileName, name)
+ return getCorrectNameOrDefault(ext, scioExt, defaultScioFileName, name), nil
+ default:
+ return name, nil
+ }
+}
+
+// isValidFileExtensionAndSDK returns a flag indicating the result of validation
+func isValidFileExtensionAndSDK(ext string, sdk pb.Sdk) bool {
+ switch ext {
+ case javaExt:
+ return sdk == pb.Sdk_SDK_JAVA
+ case goExt:
+ return sdk == pb.Sdk_SDK_GO
+ case pythonExt:
+ return sdk == pb.Sdk_SDK_PYTHON
+ case scioExt:
+ return sdk == pb.Sdk_SDK_SCIO
default:
- return name
+ return true
}
}
+// getExtBasedOnContent return a file extension
+func getExtOrExtBasedOnContent(ext, content string) string {
+ if ext == "" {
+ return getExtBasedOnContent(content)
+ }
+ return ext
+}
+
+// getExtBasedOnContent return a file extension based on the content
+func getExtBasedOnContent(content string) string {
+ if strings.Contains(content, javaMainMethod) {
+ return javaExt
+ }
+ if strings.Contains(content, goMainMethod) {
+ return goExt
+ }
+ if strings.Contains(content, pythonMainMethod) {
+ return pythonExt
+ }
+ if strings.Contains(content, goMainMethod) {
Review Comment:
goMainMethod looks incorrect here
##########
playground/backend/internal/utils/file_utils.go:
##########
@@ -35,53 +37,105 @@ const (
goExt = ".go"
pythonExt = ".py"
scioExt = ".scala"
+
+ mismatchExtAndSDKErrMsg = "file extension doesn't match to the specified SDK"
)
// GetFileName returns the valid file name.
-func GetFileName(name string, sdk pb.Sdk) string {
+func GetFileName(name, content string, sdk pb.Sdk) (string, error) {
if name == "" {
logger.Warn("The name of the file is empty. Will be used default value")
switch sdk {
case pb.Sdk_SDK_JAVA:
- return defaultJavaFileName
+ return defaultJavaFileName, nil
case pb.Sdk_SDK_GO:
- return defaultGoFileName
+ return defaultGoFileName, nil
case pb.Sdk_SDK_PYTHON:
- return defaultPythonFileName
+ return defaultPythonFileName, nil
case pb.Sdk_SDK_SCIO:
- return defaultScioFileName
+ return defaultScioFileName, nil
}
}
- return getCorrectFileName(name, sdk)
+ return getCorrectFileName(name, content, sdk)
}
// getCorrectFileName returns the correct file name.
-func getCorrectFileName(name string, sdk pb.Sdk) string {
- ext := filepath.Ext(name)
+func getCorrectFileName(name, content string, sdk pb.Sdk) (string, error) {
+ ext := getExtOrExtBasedOnContent(filepath.Ext(name), content)
+ if !isValidFileExtensionAndSDK(ext, sdk) {
+ return "", errors.New(mismatchExtAndSDKErrMsg)
+ }
switch sdk {
case pb.Sdk_SDK_JAVA:
- return getCorrectNameOrDefault(ext, javaExt, defaultJavaFileName, name)
+ return getCorrectNameOrDefault(ext, javaExt, defaultJavaFileName, name), nil
case pb.Sdk_SDK_GO:
- return getCorrectNameOrDefault(ext, goExt, defaultGoFileName, name)
+ return getCorrectNameOrDefault(ext, goExt, defaultGoFileName, name), nil
case pb.Sdk_SDK_PYTHON:
- return getCorrectNameOrDefault(ext, pythonExt, defaultPythonFileName, name)
+ return getCorrectNameOrDefault(ext, pythonExt, defaultPythonFileName, name), nil
case pb.Sdk_SDK_SCIO:
- return getCorrectNameOrDefault(ext, scioExt, defaultScioFileName, name)
+ return getCorrectNameOrDefault(ext, scioExt, defaultScioFileName, name), nil
+ default:
+ return name, nil
+ }
+}
+
+// isValidFileExtensionAndSDK returns a flag indicating the result of validation
+func isValidFileExtensionAndSDK(ext string, sdk pb.Sdk) bool {
+ switch ext {
+ case javaExt:
+ return sdk == pb.Sdk_SDK_JAVA
+ case goExt:
+ return sdk == pb.Sdk_SDK_GO
+ case pythonExt:
+ return sdk == pb.Sdk_SDK_PYTHON
+ case scioExt:
+ return sdk == pb.Sdk_SDK_SCIO
default:
- return name
+ return true
Review Comment:
Why do we have "true" here?
##########
playground/backend/internal/db/mapper/datastore_mapper_test.go:
##########
@@ -122,7 +123,7 @@ func TestEntityMapper_ToFileEntity(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
- result := testable.ToFileEntity(tt.args.info, tt.args.file)
+ result, _ := testable.ToFileEntity(tt.args.info, tt.args.file)
Review Comment:
Could you please add tests for cases where ToFileEntity returns an error?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #22792: [Playground] [Backend] added SDK validation to save a code snippet
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22792:
URL: https://github.com/apache/beam/pull/22792#issuecomment-1220689385
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] vchunikhin commented on pull request #22792: [Playground] [Backend] added SDK validation to save a code snippet
Posted by GitBox <gi...@apache.org>.
vchunikhin commented on PR #22792:
URL: https://github.com/apache/beam/pull/22792#issuecomment-1220687985
R: @pabloem
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] vchunikhin commented on a diff in pull request #22792: [Playground] [Backend] added SDK validation to save a code snippet
Posted by GitBox <gi...@apache.org>.
vchunikhin commented on code in PR #22792:
URL: https://github.com/apache/beam/pull/22792#discussion_r958349513
##########
playground/backend/cmd/server/controller_test.go:
##########
@@ -863,7 +863,7 @@ func TestPlaygroundController_SaveSnippet(t *testing.T) {
t.Errorf("PlaygroundController_SaveSnippet() error = %v, wantErr %v", err, tt.wantErr)
}
if err == nil {
- if len(got.Id) != 11 || got.Id != tt.wantId {
+ if len(got.Id) != 11 {
Review Comment:
It's the length of the identifier that is used to store snippets in the Cloud Datastore. It's appropriate length to save storage size in the Cloud Datastore and provide good randomnicity. This parameter stores in the playground/backend/properties.yaml file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem merged pull request #22792: [Playground] [Backend] added SDK validation to save a code snippet
Posted by GitBox <gi...@apache.org>.
pabloem merged PR #22792:
URL: https://github.com/apache/beam/pull/22792
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on a diff in pull request #22792: [Playground] [Backend] added SDK validation to save a code snippet
Posted by GitBox <gi...@apache.org>.
pabloem commented on code in PR #22792:
URL: https://github.com/apache/beam/pull/22792#discussion_r957581552
##########
playground/backend/cmd/server/controller_test.go:
##########
@@ -863,7 +863,7 @@ func TestPlaygroundController_SaveSnippet(t *testing.T) {
t.Errorf("PlaygroundController_SaveSnippet() error = %v, wantErr %v", err, tt.wantErr)
}
if err == nil {
- if len(got.Id) != 11 || got.Id != tt.wantId {
+ if len(got.Id) != 11 {
Review Comment:
why is the length always 11?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org