lostluck commented on a change in pull request #16153: URL: https://github.com/apache/beam/pull/16153#discussion_r766150770
########## File path: sdks/go/pkg/beam/core/runtime/xlangx/expansionx/download.go ########## @@ -0,0 +1,127 @@ +// 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 expansionx contains utilities for starting expansion services for +// cross-language transforms. +package expansionx + +import ( + "fmt" + "io" + "net/http" + "os" + "os/user" + "path/filepath" + "strings" +) + +type url string + +type jarGetter struct { + repository url + groupID string + jarCache string +} + +var defaultJarGetter = newJarGetter() + +const ( + apacheRepository = url("https://repo.maven.apache.org/maven2";) + beamGroupID = "org/apache/beam" + jarCache = "~/.apache_beam/cache/jars" +) + +func newJarGetter() *jarGetter { + return &jarGetter{repository: apacheRepository, groupID: beamGroupID, jarCache: jarCache} +} + +// GetBeamJar checks a temporary directory for the desired Beam JAR, downloads the +// appropriate JAR from Maven if not present, then returns the file path to the +// JAR. +func GetBeamJar(gradleTarget, version string) (string, error) { + return defaultJarGetter.getJar(gradleTarget, version) +} + +func (j *jarGetter) getJar(gradleTarget, version string) (string, error) { + strippedTarget := dropEndOfGradleTarget(gradleTarget) + fullURL, jarName := j.getURLForBeamJar(strippedTarget, version) + + cacheDir := j.getCacheDir() + err := os.MkdirAll(cacheDir, 0700) + if err != nil { + return "", err + } + + jarPath := filepath.Join(cacheDir, jarName) + + if jarExists(jarPath) { + return jarPath, nil + } + + resp, err := http.Get(string(fullURL)) + if err != nil { + return "", err + } + defer resp.Body.Close() + + if resp.StatusCode != 200 { + return "", fmt.Errorf("Received non 200 response code, got %v", resp.StatusCode) + } + + file, err := os.Create(jarPath) + if err != nil { + return "", err + } + + _, err = io.Copy(file, resp.Body) + if err != nil { + return "", err + } + + return jarPath, nil +} + +// getURLForBeamJar builds the Maven URL for the JAR and the JAR name, returning both +// separately so the JAR name can be used for saving the file later. +func (j *jarGetter) getURLForBeamJar(gradleTarget, version string) (url, string) { + baseURL := j.repository + url("/"+j.groupID+"/") + fullTarget := "beam" + gradleTarget + targetPath := strings.ReplaceAll(fullTarget, ":", "-") + jarName := strings.ReplaceAll(fullTarget, ":", "-") + "-" + version + ".jar" Review comment: I see what you mean, there's a bit of ordering here that can be improved, and so many " I think in this instance, fmt.Sprintf is probably marginally more efficient than the concat too. But we don't care too much about efficiency here, readability first, efficiency later. Since you don't use baseURL outside of the final URL, there's no need to do it early. And we can use the `path` package which always uses forward slashes for the joins. https://pkg.go.dev/path#Join On the whole it's probably cleaner to do this: ``` gradlePath := strings.ReplaceAll(gradleTarget, ":", "-") targetPath := "beam" + gradlePath jarName := fmt.Sprintf("beam%s-%s.jar", targetPath,version) finalURL := url(path.Join(j.repository, j.groupID, targetPath, version, jarName)) ``` Only 4 pairs of quotes too. ########## File path: sdks/go/pkg/beam/core/runtime/xlangx/expansionx/download.go ########## @@ -0,0 +1,127 @@ +// 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 expansionx contains utilities for starting expansion services for +// cross-language transforms. +package expansionx + +import ( + "fmt" + "io" + "net/http" + "os" + "os/user" + "path/filepath" + "strings" +) + +type url string + +type jarGetter struct { + repository url + groupID string + jarCache string +} + +var defaultJarGetter = newJarGetter() + +const ( + apacheRepository = url("https://repo.maven.apache.org/maven2";) + beamGroupID = "org/apache/beam" + jarCache = "~/.apache_beam/cache/jars" +) + +func newJarGetter() *jarGetter { + return &jarGetter{repository: apacheRepository, groupID: beamGroupID, jarCache: jarCache} +} + +// GetBeamJar checks a temporary directory for the desired Beam JAR, downloads the +// appropriate JAR from Maven if not present, then returns the file path to the +// JAR. +func GetBeamJar(gradleTarget, version string) (string, error) { + return defaultJarGetter.getJar(gradleTarget, version) +} + +func (j *jarGetter) getJar(gradleTarget, version string) (string, error) { + strippedTarget := dropEndOfGradleTarget(gradleTarget) + fullURL, jarName := j.getURLForBeamJar(strippedTarget, version) + + cacheDir := j.getCacheDir() + err := os.MkdirAll(cacheDir, 0700) + if err != nil { + return "", err + } + + jarPath := filepath.Join(cacheDir, jarName) + + if jarExists(jarPath) { + return jarPath, nil + } + + resp, err := http.Get(string(fullURL)) + if err != nil { + return "", err + } + defer resp.Body.Close() + + if resp.StatusCode != 200 { + return "", fmt.Errorf("Received non 200 response code, got %v", resp.StatusCode) + } + + file, err := os.Create(jarPath) + if err != nil { + return "", err + } + + _, err = io.Copy(file, resp.Body) + if err != nil { + return "", err + } + + return jarPath, nil +} + +// getURLForBeamJar builds the Maven URL for the JAR and the JAR name, returning both +// separately so the JAR name can be used for saving the file later. +func (j *jarGetter) getURLForBeamJar(gradleTarget, version string) (url, string) { + baseURL := j.repository + url("/"+j.groupID+"/") + fullTarget := "beam" + gradleTarget + targetPath := strings.ReplaceAll(fullTarget, ":", "-") + jarName := strings.ReplaceAll(fullTarget, ":", "-") + "-" + version + ".jar" + return baseURL + url(targetPath+"/"+version+"/"+jarName), jarName +} + +// getCacheDir returns the absolute file path of the JAR cache from the user's HOME +// directory. +func (j *jarGetter) getCacheDir() string { + usr, _ := user.Current() + return filepath.Join(usr.HomeDir, j.jarCache[2:]) +} + +// dropEndOfGradleTarget drops the last substring off of the gradle target. This +// is used to build the Maven target and JAR name (the last substring on the gradle) +// command is usually a directive, not a reference to the desired JAR.) +func dropEndOfGradleTarget(gradleTarget string) string { + elms := strings.Split(gradleTarget, ":") + droppedSuffix := elms[:len(elms)-1] + return strings.Join(droppedSuffix, ":") Review comment: If I understand this correctly, first we split on all the `:`, we drop the last one, and then we rejoin everything with `:`. Consider instead, finding the last :, and then slicing out the suffix. ``` i := strings.LastIndex(gradleTarget, ":") return gradleTarget[:i] ``` ``` ########## File path: sdks/go/pkg/beam/core/runtime/xlangx/expansionx/download.go ########## @@ -0,0 +1,127 @@ +// 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 expansionx contains utilities for starting expansion services for +// cross-language transforms. +package expansionx + +import ( + "errors" + "fmt" + "io" + "net/http" + "os" + "os/user" + "path/filepath" + "strings" +) + +// URL is a type used to differentiate between strings and URLs +type URL string + +const ( + apacheRepository = URL("https://repo.maven.apache.org/maven2";) + beamGroupID = "org/apache/beam" + jarCache = "~/.apache_beam/cache/jars" +) + +// GetBeamJar checks a temporary directory for the desired Beam JAR, downloads the +// appropriate JAR from Maven if not present, then returns the file path to the +// JAR. +func GetBeamJar(gradleTarget, version string) (string, error) { Review comment: No need to change, since everything's unexported, but probably unnecessary in this instance to have a "default instance" and can probably define in place instead. We needed the struct approach to make it more easily testable, but since we don't have piles of state and multiple functions to keep track of, defining it in place makes it easier to see what's being used by the call. This is the principle of Least Mechanism. Only add as much as you need, and not much more. Makes things easier to read for later modifiers, if not necessarily having things in place already. To emphasize, no need to change anything for this comment, it's just a teachable moment. ########## File path: sdks/go/pkg/beam/core/runtime/xlangx/expansionx/download_test.go ########## @@ -0,0 +1,101 @@ +// 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 expansionx + +import ( + "io/ioutil" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestGetURLForBeamJar(t *testing.T) { + tests := []struct { + name string + target string + version string + expectedURL url + }{ + { + "base", + ":sdks:java:fake", + "VERSION", + "https://repo.maven.apache.org/maven2/org/apache/beam/beam-sdks-java-fake/VERSION/beam-sdks-java-fake-VERSION.jar";, + }, + { + "versioned", + ":sdks:java:fake", + "1.2.3", + "https://repo.maven.apache.org/maven2/org/apache/beam/beam-sdks-java-fake/1.2.3/beam-sdks-java-fake-1.2.3.jar";, + }, + } + for _, test := range tests { + j := defaultJarGetter Review comment: Use the construction function for the default here, rather than testing against the default. This way lies non-hermetic tests. If you have a concern, you can always have one test check that the defaultJarGetter is equal to the one in the construction function. -- 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]
