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 12:51:43 UTC
[GitHub] [beam] olehborysevych commented on a diff in pull request #22792: [Playground] [Backend] added SDK validation to save a code snippet
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