damondouglas commented on a change in pull request #15660:
URL: https://github.com/apache/beam/pull/15660#discussion_r722654834
##########
File path: playground/backend/internal/executors/executor.go
##########
@@ -13,19 +13,20 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-// Interface for all executors (Java/Python/Go/SCIO)
+// Package executors
package executors
-type executor interface {
- // Validate validates executable file.
+// Executor interface for all executors (Java/Python/Go/SCIO)
+type Executor interface {
Review comment:
May we consider breaking this into three interfaces `Validater`,
`Compiler`, and `Runner` with `Validate`, `Compile`, and `Run` methods?
##########
File path: playground/backend/internal/executors/executor.go
##########
@@ -13,19 +13,20 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-// Interface for all executors (Java/Python/Go/SCIO)
+// Package executors
package executors
-type executor interface {
- // Validate validates executable file.
+// Executor interface for all executors (Java/Python/Go/SCIO)
+type Executor interface {
+ // Validate checks that the file exists and that extension of the file
matches the SDK.
// Return result of validation (true/false) and error if it occurs
- Validate(filePath string) (bool, error)
+ Validate(fileName string) (bool, error)
- // Compile compiles executable file.
+ // Compile compiles the code and creates executable file.
// Return error if it occurs
- Compile(filePath string) error
+ Compile(fileName string) error
- // Run runs executable file.
+ // Run runs the executable file.
// Return logs and error if it occurs
- Run(filePath string) (string, error)
+ Run(fileName string) (string, error)
Review comment:
Could we look to how the https://pkg.go.dev/os/exec package designs its
codebase for insight into the method signatures? The code seems to follow a
configuration constructor like `Command`, where the `Cmd` struct stores
internally what it will be executing. I wonder if it even just makes sense to
have a Command method that uses the https://pkg.go.dev/os/exec package
internally and returns `Cmd` instead of implementing our own `Run` method.
##########
File path: playground/backend/internal/executors/java_executor.go
##########
@@ -0,0 +1,72 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package executors
+package executors
+
+import (
+ "beam.apache.org/playground/backend/internal/fs_tool"
+ "fmt"
+ "os/exec"
+ "path/filepath"
+ "strings"
+)
+
+const (
+ beamJarPath = "/opt/apache/beam/jars/beam-sdks-java-harness.jar"
+ runnerJarPath = "/opt/apache/beam/jars/beam-runners-direct.jar"
+ slf4jPath = "/opt/apache/beam/jars/slf4j-jdk14.jar"
+ javaExtension = ".java"
+)
+
+type CompileError struct {
+ error string
+}
+
+func (e *CompileError) Error() string {
+ return fmt.Sprintf("Compilation error: %v", e.error)
+}
+
+// JavaExecutor for Java code
+type JavaExecutor struct {
+ fs fs_tool.JavaFileSystemService
+}
+
+func (javaExec *JavaExecutor) Validate(fileName string) (bool, error) {
+ filePath := filepath.Join(javaExec.fs.GetSrcPath(), fileName)
+ return fs_tool.CheckPathIsValid(filePath, javaExtension)
+}
+
+func (javaExec *JavaExecutor) Compile(fileName string) error {
+ cmd := exec.Command("javac", "-d", javaExec.fs.GetBinPath(),
"-classpath", beamJarPath,
+ filepath.Join(javaExec.fs.GetSrcPath(), fileName))
+ out, err := cmd.CombinedOutput()
+ if err != nil {
+ return &CompileError{string(out)}
+ }
+ return nil
+}
+
+func (javaExec *JavaExecutor) Run(className string) (string, error) {
Review comment:
See comment above regarding the method signature. Perhaps it makes
sense to have a Command method that returns a `*Cmd`.
##########
File path: playground/backend/internal/fs_tool/path_checker.go
##########
@@ -0,0 +1,50 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package fs_tool utils for checking the valid file path
+package fs_tool
+
+import (
+ "os"
+ "path/filepath"
+ "strings"
+)
+
+// IsExist checks if file exists
+func IsExist(filePath string) (bool, error) {
+ if _, err := os.Stat(filePath); err == nil {
+ return true, nil
+ } else if os.IsNotExist(err) {
+ return false, nil
+ } else {
+ return false, err
+ }
+}
+
+// IsCorrectExtension checks if the file has correct extension (.java, .go,
.py)
+func IsCorrectExtension(filePath string, correctExtension string) bool {
Review comment:
Should this be one of the list of validators if we are using the
multiple validators approach to the relevant life cycle stage?
##########
File path: playground/backend/internal/executors/executor.go
##########
@@ -13,19 +13,20 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-// Interface for all executors (Java/Python/Go/SCIO)
+// Package executors
package executors
-type executor interface {
- // Validate validates executable file.
+// Executor interface for all executors (Java/Python/Go/SCIO)
+type Executor interface {
+ // Validate checks that the file exists and that extension of the file
matches the SDK.
// Return result of validation (true/false) and error if it occurs
- Validate(filePath string) (bool, error)
+ Validate(fileName string) (bool, error)
Review comment:
Should we just return `error`? Returning `nil` in this setting would
perhaps mean that the validation passes? Additionally, should we have the
fileName be a private property of the struct implementing the Validate method?
There may be other validations of the system state where information may be
needed more than just the fileName possibly.
##########
File path: playground/backend/internal/executors/java_executor.go
##########
@@ -0,0 +1,72 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package executors
+package executors
+
+import (
+ "beam.apache.org/playground/backend/internal/fs_tool"
+ "fmt"
+ "os/exec"
+ "path/filepath"
+ "strings"
+)
+
+const (
+ beamJarPath = "/opt/apache/beam/jars/beam-sdks-java-harness.jar"
+ runnerJarPath = "/opt/apache/beam/jars/beam-runners-direct.jar"
+ slf4jPath = "/opt/apache/beam/jars/slf4j-jdk14.jar"
+ javaExtension = ".java"
+)
+
+type CompileError struct {
+ error string
+}
+
+func (e *CompileError) Error() string {
+ return fmt.Sprintf("Compilation error: %v", e.error)
+}
+
+// JavaExecutor for Java code
+type JavaExecutor struct {
+ fs fs_tool.JavaFileSystemService
+}
+
+func (javaExec *JavaExecutor) Validate(fileName string) (bool, error) {
+ filePath := filepath.Join(javaExec.fs.GetSrcPath(), fileName)
+ return fs_tool.CheckPathIsValid(filePath, javaExtension)
Review comment:
I realize that firewall rules will prevent potential security breaches
but should we consider additional redundant security checks as part of the
validation? For example, pattern matching certain package imports or classes
that may be used to make network calls? Should their be checks against an
allowed pattern of imports? Multiple layers of security may be helpful to
mitigate the risk of this application. This relates to the signature of this
method suggested above in the interface section as being empty and allowing for
potentially multiple and combined validations.
##########
File path: playground/backend/internal/executors/go_executor.go
##########
@@ -13,19 +13,20 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-// Executor for Go
+// Package executors
package executors
+// GoExecutor for Go code
type GoExecutor struct{}
Review comment:
Would it make sense to have the struct agnostic to what it is executing
and have a single struct that will implement the life cycle stages - validate,
compile, and run? The language specifics may be specific helpers in the
configuration of this one executer struct maybe. This then questions whether
even having an interface makes sense if there will be only a single struct that
implements the life cycle stages.
##########
File path: playground/backend/internal/fs_tool/path_checker.go
##########
@@ -0,0 +1,50 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package fs_tool utils for checking the valid file path
+package fs_tool
+
+import (
+ "os"
+ "path/filepath"
+ "strings"
+)
+
+// IsExist checks if file exists
+func IsExist(filePath string) (bool, error) {
Review comment:
Instead of `IsExist`, may we consider the following:
```
func IsNotExist(filePath string) bool {
_, err := os.Stat(filePath)
return errors.Is(err, fs.ErrNotExist)
}
```
See https://pkg.go.dev/os#IsNotExist for details.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
