Fixes #1439 - Verify if we need to handle proxy settings in com.google.cloud.tools.eclipse.projectselector.GoogleApiFactory by akerekes · Pull Request #1513 · GoogleCloudPlatform/google-cloud-eclipse

akerekes · 2017-03-02T21:07:50Z

I've turned GoogleApiFactory into a service.
A proxy settings change listener is used to ensure we respect those changes.

A test for proxy setting changes is still pending, but the rest is ready for review.

…from IProxyService

chanseokoh

Just some quick comments.

chanseokoh · 2017-03-02T21:13:31Z

+@Component
+public class GoogleApiFactory implements IGoogleApiFactory {

+  private static final String GOOGLEAPIS_URL = "https://appengine.googleapis.com";


It doesn't feel right that this one contains appengine.

chanseokoh · 2017-03-02T21:18:49Z

 import com.google.api.client.http.javanet.NetHttpTransport;
 import com.google.api.client.json.JsonFactory;
 import com.google.api.client.json.jackson.JacksonFactory;
+import com.google.api.client.repackaged.com.google.common.base.Preconditions;


This is from a wrong dependency.

chanseokoh · 2017-03-02T21:20:48Z

+    Preconditions.checkNotNull(proxyService, "proxyService is null");
+    jsonFactory = new JacksonFactory();
+    buildTransport();
+    proxyService.addProxyChangeListener(new IProxyChangeListener() {


I'm concerned about thread-safety here. It's not a UI widget, so I think this can be executed in any thread.

elharo

Looks good after Chanseok's comments are addressed

… URL

codecov-io · 2017-03-02T23:03:43Z

Codecov Report

Merging #1513 into master will decrease coverage by -0.03%.
The diff coverage is 65.21%.

@@             Coverage Diff              @@
##             master    #1513      +/-   ##
============================================
- Coverage      70.3%   70.28%   -0.03%     
- Complexity     1299     1300       +1     
============================================
  Files           232      232              
  Lines          8935     8942       +7     
  Branches        762      761       -1     
============================================
+ Hits           6282     6285       +3     
- Misses         2335     2339       +4     
  Partials        318      318

Impacted Files	Coverage Δ	Complexity Δ
...gine/deploy/ui/StandardDeployPreferencesPanel.java	`85.14% <ø> (ø)`	`22 <0> (ø)`	❌
...ploy/ui/standard/StandardDeployCommandHandler.java	`1.38% <0%> (-0.04%)`	`1 <0> (ø)`
...e/appengine/deploy/ui/DeployPreferencesDialog.java	`0% <0%> (ø)`	`0 <0> (ø)`	❌
...d/tools/eclipse/test/util/http/TestHttpServer.java	`91.83% <100%> (+0.34%)`	`12 <2> (+2)`	✅
...clipse/appengine/deploy/ui/DeployPropertyPage.java	`52.74% <100%> (+1.06%)`	`13 <0> (ø)`	❌
...ols/eclipse/projectselector/ProjectRepository.java	`94.59% <81.81%> (-0.15%)`	`14 <1> (-1)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39c0969...5ac0152. Read the comment docs.

chanseokoh · 2017-03-02T23:26:32Z

+        buildCloudResourceManagerTransport();
+      } catch (URISyntaxException ex) {
+        logger.log(Level.SEVERE, "Could not create transport using the proxy settings", ex);
+        transports = null;


I think this will cause NPE in many places onward. Even if you initialize it with an empty map, it will cause trouble in places where you do transports.get(...). Probably it's better to leave transport as-is? At least it will keep previous, working transports.

Speaking of which, should we be prepared for when transports.get(...) returns null too?

chanseokoh · 2017-03-02T23:37:37Z

+      CloudResourceManager resourceManager =
+          new CloudResourceManager.Builder(transports.get(googleApiUrls.cloudResourceManagerUrl()),
+                                           jsonFactory, credential)
+          .setApplicationName(CloudToolsInfo.USER_AGENT).build();


Nit: indent +4

chanseokoh · 2017-03-02T23:37:45Z

+      Appengine appengine =
+          new Appengine.Builder(transports.get(googleApiUrls.appEngineAdminUrl()), jsonFactory,
+                                credential)
+          .setApplicationName(CloudToolsInfo.USER_AGENT).build();


Nit: indent +4

briandealwis · 2017-03-03T15:10:47Z

+  private static final Logger logger = Logger.getLogger(GoogleApiFactory.class.getName());
+
+  private JsonFactory jsonFactory;
+  private Map<String, HttpTransport> transports = new HashMap<>();


Are these HttpTransports expensive? Do we really want them living forever? Would we be better off making this a Guava Cache instead?

The reason to keep them around is because of the Javadoc of NetHttpTransport:

Implementation is thread-safe. For maximum efficiency, applications should use a single globally-shared instance of the HTTP transport.

It's your call, but I'd prefer to have them GC'd when they're unused. Guava Caches are thread-safe, so we can probably get rid of synchronization here, and the CacheLoader will simplify some of the logic here. And using weakValues() means the instances will stick around as long as they're being used.

briandealwis · 2017-03-03T15:22:35Z

+
+    IProxyData[] proxyData = proxyService.select(new URI(url));
+    for (final IProxyData iProxyData : proxyData) {
+      if (IProxyData.HTTPS_PROXY_TYPE.equals(iProxyData.getType())) {


Not supporting SOCKS proxies? Hmm, we should log something if the IProxyService returned some proxy data (i.e., we should use a proxy) and we're not supporting it — otherwise we silently fail.

briandealwis · 2017-03-03T15:23:19Z

+  private Proxy createProxy(String url) throws URISyntaxException {
+    Preconditions.checkNotNull(proxyService, "proxyService is null");
+    Preconditions.checkNotNull(googleApiUrls, "googleApiUrl is null");
+


Since none of the Google APIs support HTTP anymore (right?) then we could assert here that the URL does not start with 'http://', so we should never see a proxy-data of type HTTP.

briandealwis · 2017-03-03T15:36:18Z

+    Preconditions.checkNotNull(proxyService, "proxyService is null");
+    jsonFactory = new JacksonFactory();
+    buildTransports();
+    proxyService.addProxyChangeListener(new IProxyChangeListener() {


I think this would be better if we made this service optionally dependent on the IProxyService (i.e., ReferenceCardinality.OPTIONAL: 0..1). If there is no proxy service, we just treat everything like DIRECT. (I believe the IProxyService is actually registered programmatically in org.eclipse.core.net's activator and so if that hasn't been activated — which is unlikely, but possible — then our service won't be available.)

…e SOCKS proxy, error on http schema

briandealwis · 2017-03-03T18:31:49Z

+        case IProxyData.SOCKS_PROXY_TYPE:
+          return new Proxy(Type.SOCKS, new InetSocketAddress(iProxyData.getHost(),
+                                                             iProxyData.getPort()));
+        default:


This code isn't handling a proxy directive, and we should at least log something.

In fact, in the case of the unhandled, we should continue looping through any other proxyData items (e.g., IProxyService returns { HTTP_PROXY_TYPE, HTTPS_PROXY_TYPE } — we don't do the first, but we will do the second). The default case should happen outside of the for loop.

briandealwis · 2017-03-03T18:35:37Z

+    }

    IProxyData[] proxyData = proxyService.select(new URI(url));
    for (final IProxyData iProxyData : proxyData) {


plural: proxyDatum? Can we come up with a better name than iProxyData?

I think @briandealwis was suggesting iProxyData --> proxyDatum. (Technically Datum is the singular form, but I guess nowadays people just use data for anything.)

I was referring to iProxyData. And oops re: datum being singular!

briandealwis · 2017-03-03T20:37:52Z

               plugin.properties,\
-               lib/
+               lib/,\
+               OSGI-INF/com.google.cloud.tools.eclipse.googleapis.internal.GoogleApiFactory.xml


Might just want to make this OSGI-INF/ rather than the specific file.

briandealwis · 2017-03-03T20:51:59Z

+    Preconditions.checkNotNull(googleApiUrls, "googleApiUrl is null");
+    Preconditions.checkArgument(!url.startsWith("http://"), "http is not supported schema");
+
+    if (proxyService == null) {


I'm sorry for suggesting you make this dynamic. You should operate on a copy of the proxyService reference as it may be unbound before the call to select() below.

briandealwis · 2017-03-03T20:58:59Z

+  @Reference(policy=ReferencePolicy.DYNAMIC, cardinality=ReferenceCardinality.OPTIONAL)
+  public void setProxyService(IProxyService proxyService) {
+    this.proxyService = proxyService;
+    this.proxyService.addProxyChangeListener(proxyChangeListener);


I'm pretty sure OSGi ensures you won't be unbound while being bound, so this field access is safe.

akerekes · 2017-03-03T23:15:36Z

@briandealwis @elharo @chanseokoh PTAL
FYI tests are coming, but functionality-wise it's ready for another round of review.

briandealwis

Looking good!

briandealwis · 2017-03-04T03:59:29Z

+
+package com.google.cloud.tools.eclipse.googleapis;
+
+public class GoogleApiException extends Exception {


Javadoc would be good.

briandealwis · 2017-03-04T04:02:59Z

    this.proxyService = proxyService;
    this.proxyService.addProxyChangeListener(proxyChangeListener);
-    buildTransports();
+    if (proxyFactory != null) {


Is proxyFactory ever null? Would it make sense to make it final and set in the constructor?

chanseokoh

Some quick readability suggestions.

chanseokoh · 2017-03-06T21:03:46Z

+  }
+
+  private void connectReadAndDisconnect(HttpURLConnection connection) throws IOException {
+    connection.connect();


I think it's concise and also better to just check

assertThat(connection.getResponseCode(), is(200));

and remove the try block altogether (while leaving the finally block.)

My intent was to check the response and fail if we accidentally connected to an existing site. I've changed the code to make this more explicit.

chanseokoh · 2017-03-06T21:08:44Z

+  };
+
+  public GoogleApiFactory() {
+    proxyFactory = new ProxyFactory();


Personally, I prefer this(new ProxyFactory()). This way, it's easy to tell if there are difference between the two constructors, and if so, how they differ.

chanseokoh · 2017-03-06T21:09:35Z

+
+class TransportCacheLoader extends CacheLoader<GoogleApiUrl, HttpTransport> {
+
+  private ProxyFactory proxyFactory;


Could be final.

chanseokoh · 2017-03-06T21:16:29Z

      // used to indicate a missing application
      return AppEngine.withId(application.getId());
-    } catch (IOException ex) {
+    } catch (IOException | GoogleApiException ex) {


Now we're at it, I think it's better to

} catch (GoogleJsonResponseException ex) { ... } catch (IOException | GoogleApiException ex) { ... }

and remove the if (ex instanceof GoogleJsonResponseException) {.

chanseokoh · 2017-03-06T21:19:48Z

+  APPENGINE_ADMIN_API("https://appengine.googleapis.com"),
+  CLOUDRESOURCE_MANAGER_API("https://cloudresourcemanager.googleapis.com");
+
+  private String url;


briandealwis · 2017-03-07T04:37:45Z


-public class GoogleApiFactoryTest {
+/**
+ * 


Should be filled in

briandealwis · 2017-03-07T04:41:12Z

+
+  @Activate
+  public void init() {
+    jsonFactory = new JacksonFactory();


Any reason you're not initializing this and making it final? You set mocks in the tests, but don't seem to do anything with the mocks.

briandealwis · 2017-03-07T04:42:41Z

 */
 class TimeoutAwareConnectionFactory extends DefaultConnectionFactory {
+
+  private static final int DEFAULT_TIMEOUT_MS = 1000;


I wonder if this should be a tuneable parameter.

We can change it when it becomes necessary.

…leApiFactory

akerekes added 3 commits March 1, 2017 13:53

make GoogleApiFactory a service, create proxy based on info obtained …

aaec17f

…from IProxyService

Merge branch 'master' into i1439

79b8a43

finish turning GoogleApiFactory into a service, add some tests

95d77e7

akerekes requested review from briandealwis and elharo March 2, 2017 21:07

googlebot added the cla: yes label Mar 2, 2017

chanseokoh reviewed Mar 2, 2017

View reviewed changes

elharo reviewed Mar 2, 2017

View reviewed changes

make transport access/modification synchronized, create transport per…

34cb361

… URL

chanseokoh reviewed Mar 2, 2017

View reviewed changes

briandealwis reviewed Mar 3, 2017

View reviewed changes

akerekes added 2 commits March 3, 2017 11:49

make IProxyService an optional dependency, handle if it's null, handl…

b2b3196

…e SOCKS proxy, error on http schema

remove proxy change listener when unsetting proxy service

61335ee

briandealwis reviewed Mar 3, 2017

View reviewed changes

fix proxy selection, rename local and loop variables

0ed0693

briandealwis reviewed Mar 3, 2017

View reviewed changes

akerekes added 2 commits March 3, 2017 18:13

use Cache instead of Map for transports in GoogleApiFactory

10155bb

simplify build.properties

35d943b

briandealwis reviewed Mar 4, 2017

View reviewed changes

add more tests to Google APIs bundle

361b318

chanseokoh reviewed Mar 6, 2017

View reviewed changes

review comments addressed

e66bb0d

chanseokoh approved these changes Mar 6, 2017

View reviewed changes

briandealwis reviewed Mar 7, 2017

View reviewed changes

akerekes added 2 commits March 7, 2017 10:26

fill in javadoc for IGoogleApiFactory, make jsonFactory final in Goog…

b8e5f00

…leApiFactory

Merge branch 'master' into i1439

5ac0152

akerekes merged commit 78772ab into master Mar 7, 2017

akerekes deleted the i1439 branch March 7, 2017 18:04


		package com.google.cloud.tools.eclipse.googleapis;

		public class GoogleApiException extends Exception {


		class TransportCacheLoader extends CacheLoader<GoogleApiUrl, HttpTransport> {

		private ProxyFactory proxyFactory;

Conversation

akerekes commented Mar 2, 2017

Uh oh!

chanseokoh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chanseokoh Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akerekes commented Mar 3, 2017

Uh oh!

briandealwis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chanseokoh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov-io commented Mar 2, 2017 •

edited

Loading

chanseokoh Mar 3, 2017 •

edited

Loading