Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Friendly Provider determination #21

Merged
merged 18 commits into from
Apr 6, 2016
Merged

Friendly Provider determination #21

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ccc3e54
ProviderScanner interface & UserAgentImpl stubs
Mar 22, 2016
dc6bd62
Extract scanProviders & describeUnmetRequirements
Mar 22, 2016
ff1b161
Move join() to new StringHelper class
Mar 22, 2016
1f05e3e
Add null-aware StringHelper.equal() method
Mar 22, 2016
19719d8
Introduce constant for future re-use
Mar 22, 2016
f5d72d4
Collect candidate Provider instances in ctor
Mar 23, 2016
1c8516b
Barely implement getUnmetProviderRequirements()
Mar 23, 2016
7cf7b06
Initial implementation of findCompatibleProvider()
Mar 23, 2016
c03e4b8
Add caching to avoid unnecessary scans
Mar 23, 2016
0d09fc0
Verify number of checks in existing tests
Mar 23, 2016
deef682
Verify 2 identical calls to find mean only 1 check
Mar 23, 2016
2d58dc5
getUnmetProviderRequirements should have >= 1 scan
Mar 23, 2016
a88c564
Implement rest of ProviderScanner interface
Mar 23, 2016
462c4cc
Extracted throwUnsupported() method
Mar 23, 2016
d731ba0
Update test that was just exercising scanProviders
Mar 23, 2016
1bcfcdf
Update test to be more precise
Mar 23, 2016
329cb53
Switch to new implementation & delete dead code
Mar 23, 2016
b7db17f
Fixed missing compatibility check in new methods
Apr 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions oauth2-useragent-core/src/main/java/com/microsoft/alm/oauth2/useragent/ProviderScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root.

package com.microsoft.alm.oauth2.useragent;

import java.util.List;
import java.util.Map;

public interface ProviderScanner {

/**
* Scans the {@link Provider} implementations, checks their requirements
* and tries to find one that will work, preferring the one named by the
* {@code userAgentProvider} property, if it was set.
*
* @return a {@link Provider} that was deemed compatible;
* {@code null} if no compatible providers were found.
*/
Provider findCompatibleProvider();

/**
* Scans the {@link Provider} implementations, checks their requirements
* and tries to find one that will work, preferring the one whose
* name was provided.
*
* @param userAgentProvider the name of the preferred provider or
* {@code null} if there's no preference
* @return a {@link Provider} that was deemed compatible;
* {@code null} if no compatible providers were found.
*/
Provider findCompatibleProvider(final String userAgentProvider);

/**
* Determines unmet requirements (if any) for each available {@link Provider} implementation.
*
* @return a map of {@link Provider} instances to their list of unmet requirements.
* If a {@link Provider} had all its requirements met, it will not be part of the map.
* An empty map means all providers are compatible.
*/
Map<Provider, List<String>> getUnmetProviderRequirements();

/**
* Calls {@link #findCompatibleProvider()} and
* returns whether a compatible {@link Provider} was found.
*
* @return {@code true} if a compatible provider was found;
* {@code false} otherwise.
*/
boolean hasCompatibleProvider();

/**
* Calls {@link #findCompatibleProvider(String)} and
* returns whether a compatible {@link Provider} was found.
*
* @param userAgentProvider the name of the preferred provider or
* {@code null} if there's no preference
* @return {@code true} if a compatible provider was found;
* {@code false} otherwise.
*/
boolean hasCompatibleProvider(final String userAgentProvider);

}
137 changes: 92 additions & 45 deletions oauth2-useragent-core/src/main/java/com/microsoft/alm/oauth2/useragent/UserAgentImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.microsoft.alm.oauth2.useragent.subprocess.TestableProcess;
import com.microsoft.alm.oauth2.useragent.subprocess.TestableProcessFactory;
import com.microsoft.alm.oauth2.useragent.utils.PackageLocator;
import com.microsoft.alm.oauth2.useragent.utils.StringHelper;

import java.io.BufferedReader;
import java.io.File;
Expand All @@ -25,19 +26,21 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.Set;

public class UserAgentImpl implements UserAgent {
public class UserAgentImpl implements UserAgent, ProviderScanner {

static final String REQUEST_AUTHORIZATION_CODE = "requestAuthorizationCode";
static final String JAVA_VERSION_STRING = System.getProperty("java.version");
static final String JAVA_HOME = System.getProperty("java.home");
static final String PATH_SEPARATOR = System.getProperty("path.separator");
static final String NEW_LINE = System.getProperty("line.separator");
static final String UTF_8 = "UTF-8";
static final String USER_AGENT_PROVIDER_PROPERTY_NAME = "userAgentProvider";

private static final String[] EMPTY_STRING_ARRAY = new String[0];
private static final Set<String> NETWORKING_PROPERTY_NAMES;
Expand Down Expand Up @@ -81,17 +84,62 @@ public class UserAgentImpl implements UserAgent {
}

private final TestableProcessFactory processFactory;
private final List<Provider> candidateProviders;
private final LinkedHashMap<Provider, List<String>> requirementsByProvider = new LinkedHashMap<Provider, List<String>>();
private Provider provider;
private boolean hasScannedAtLeastOnce = false;
private String userAgentProvider = null;

public UserAgentImpl() {
this(new DefaultProcessFactory(), null);
this(new DefaultProcessFactory(), null, Provider.PROVIDERS);
}

UserAgentImpl(final TestableProcessFactory processFactory, final Provider provider) {
UserAgentImpl(final TestableProcessFactory processFactory, final Provider provider, final List<Provider> candidateProviders) {
this.processFactory = processFactory;
this.candidateProviders = candidateProviders;
this.provider = provider;
}

@Override
public Provider findCompatibleProvider() {
final String userAgentProvider = System.getProperty(USER_AGENT_PROVIDER_PROPERTY_NAME);
return findCompatibleProvider(userAgentProvider);
}

@Override
public Provider findCompatibleProvider(final String userAgentProvider) {
if (provider == null || !StringHelper.equal(this.userAgentProvider, userAgentProvider)) {
requirementsByProvider.clear();
provider = scanProviders(userAgentProvider, candidateProviders, requirementsByProvider);
hasScannedAtLeastOnce = true;
this.userAgentProvider = userAgentProvider;
}
return provider;
}

@Override
public Map<Provider, List<String>> getUnmetProviderRequirements() {
if (!hasScannedAtLeastOnce) {
findCompatibleProvider();
}

final Map<Provider, List<String>> copy = new LinkedHashMap<Provider, List<String>>(requirementsByProvider);
final Map<Provider, List<String>> result = Collections.unmodifiableMap(copy);
return result;
}

@Override
public boolean hasCompatibleProvider() {
findCompatibleProvider();
return provider != null;
}

@Override
public boolean hasCompatibleProvider(final String userAgentProvider) {
findCompatibleProvider(userAgentProvider);
return provider != null;
}

@Override
public AuthorizationResponse requestAuthorizationCode(final URI authorizationEndpoint, final URI redirectUri)
throws AuthorizationException {
Expand All @@ -104,9 +152,9 @@ AuthorizationResponse encode(final String methodName, final String... parameters
final ArrayList<String> classPath = new ArrayList<String>();
// TODO: should we append ".exe" on Windows?
command.add(new File(JAVA_HOME, "bin/java").getAbsolutePath());
findCompatibleProvider();
if (provider == null) {
final String userAgentProvider = System.getProperty("userAgentProvider");
provider = determineProvider(userAgentProvider);
throwUnsupported(requirementsByProvider);
}
provider.augmentProcessParameters(command, classPath);

Expand Down Expand Up @@ -157,70 +205,69 @@ void addClassPathToCommand(final List<String> classPath, final List<String> comm
command.add("-classpath");
//noinspection ToArrayCallWithZeroLengthArrayArgument
final String[] classPathComponents = classPath.toArray(EMPTY_STRING_ARRAY);
final String classPathString = join(pathSeparator, classPathComponents);
final String classPathString = StringHelper.join(pathSeparator, classPathComponents);
command.add(classPathString);
}

static String join(final String separator, final String[] value)
{
if (value == null)
throw new IllegalArgumentException("value is null");

// "If separator is null, an empty string (String.Empty) is used instead."
final String sep = separator == null ? "" : separator;
static void throwUnsupported(final Map<Provider, List<String>> unmetRequirements) {
final StringBuilder sb = new StringBuilder("I don't support your platform yet.");
describeUnmetRequirements(unmetRequirements, sb);
sb.append(NEW_LINE);
sb.append("Please send details about your operating system version, Java version, 32- vs. 64-bit, etc.");
sb.append(NEW_LINE);
sb.append("The following System Properties and Environment Variables would be very useful.");
sb.append(NEW_LINE);

final StringBuilder result = new StringBuilder();
final Properties properties = System.getProperties();
appendProperties(properties, sb);
sb.append(NEW_LINE);

if (value.length > 0) {
result.append(value[0] == null ? "" : value[0]);
for (int i = 1; i < value.length; i++) {
result.append(sep);
result.append(value[i] == null ? "" : value[i]);
}
}
final Map<String, String> variables = System.getenv();
appendVariables(variables, sb);

return result.toString();
throw new IllegalStateException(sb.toString());
}

static Provider determineProvider(final String userAgentProvider) {
return determineProvider(userAgentProvider, Provider.PROVIDERS);
}
static Provider scanProviders(final String userAgentProvider, final List<Provider> providers, final Map<Provider, List<String>> destinationUnmetRequirements) {

static Provider determineProvider(final String userAgentProvider, final List<Provider> providers) {
Provider result = null;

if (userAgentProvider != null) {
for (final Provider provider : providers) {
if (provider.getClassName().equals(userAgentProvider)) {
return provider;
result = provider;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if my understanding isn't right.

According to the Javadoc findCompatibleProvider(final String userAgentProvider) will find a provider that works, only preferring the provider specified as userAgentProvider. However in this case, wouldn't this always return userAgentProvider regardless? For example, if I prefer JavaFX provider, I could always make a call findCompatibleProvider(javaFxProvider) and I will always get that provider, even though on some platform it doesn't work? From reading the Javadoc, I was under the impression that it will check if JavaFXProvider meets all the requirement, and only return it when it works.

Also another general comment, how expensive are those checks? It seems we are adding many states to UserAgentImpl because of this change, I am wondering if the checks are cheap, can we make it stateless? We could just call scanProvider() every time and check the destinationUnmetRequirements? It probably will make the code easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the Javadoc findCompatibleProvider(final String userAgentProvider) will find a provider that works, only preferring the provider specified as userAgentProvider. However in this case, wouldn't this always return userAgentProvider regardless?

You're correct: I preserved the override behaviour but the documentation to findCompatibleProvider() makes it sound like a requirements check will always be performed.

It seems to me that I can preserve the existing behaviour (while also making my documentation accurate) by adding a checkOverrideIsCompatible parameter to the determineProvider() method, such that encode() will supply false and findCompatibleProvider() will supply true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also another general comment, how expensive are those checks?

Given the recently-added issue #22, it appears the checkRequirements() for JavaFX will most likely need to try to instantiate a WebView, making the check expensive. I suspect an SWT provider might also need to do the same, given that some third-party dependencies sometimes need to be installed separately.

When I first wrote checkRequirements() and determineProvider(), there didn't seem to be any fixes you could make without restarting the process.

For example:

  • You're running the program via a remote shell - there's no desktop, so you need to start over from the desktop.
  • You've launched from an installation of Java that doesn't have JavaFX, so you need to launch from another installation of Java, possibly after downloading and installing it first.

Otherwise, once you're deemed compatible (or incompatible), it's unlikely to stop (or start) working, so there's no need to keep checking.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Using my personal account since I don't want to do 2FA)
I personally prefer not to complicate this further. What I had in mind was a simple check that the consumer calls and can get a true / false decision. Everything else is icing on the cake - I don't think they are essential to my use case.

I will leave it up to you to decide what kind of checks you provide, but I don't oppose just change the Javadoc to make the description accurate. :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, once you're deemed compatible (or incompatible), it's unlikely to stop (or start) working, so there's no need to keep checking.

It's understood that there is no need to keep checking, the comment is more about code structure -- remove states IMHO will make the code easier to read.

"Expensive" is relative, the process we are about to invoke is a user interaction, which could run in minutes. As long as the check is tolerable (sub second?), it may not matter that much.

Another option is instead of "true/false", how about determineProvider returns the compatible provider (null to signify no provider is compatible), and then consumer can pass this Provider to requestAuthorizationCode, or create a new function named requestAuthorizationCodeWithProvider and just use it. This way you might be able to avoid storing state?

break;
}
}
}
final StringBuilder sb = new StringBuilder("I don't support your platform yet.");

for (final Provider provider : providers) {
final List<String> requirements = provider.checkRequirements();
if (requirements == null || requirements.size() == 0) {
return provider;
if (result == null) {
result = provider;
}
}
sb.append(NEW_LINE);
sb.append("Unmet requirements for the '").append(provider.getClassName()).append("' provider:").append(NEW_LINE);
for (final String requirement : requirements) {
sb.append(" - ").append(requirement).append(NEW_LINE);
else {
destinationUnmetRequirements.put(provider, requirements);
}
}
sb.append(NEW_LINE);
sb.append("Please send details about your operating system version, Java version, 32- vs. 64-bit, etc.");
sb.append(NEW_LINE);
sb.append("The following System Properties and Environment Variables would be very useful.");
sb.append(NEW_LINE);

final Properties properties = System.getProperties();
appendProperties(properties, sb);
sb.append(NEW_LINE);

final Map<String, String> variables = System.getenv();
appendVariables(variables, sb);
return result;
}

throw new IllegalStateException(sb.toString());
static void describeUnmetRequirements(final Map<Provider, List<String>> unmetRequirements, final StringBuilder destination) {
for (final Map.Entry<Provider, List<String>> pair: unmetRequirements.entrySet()) {
final Provider provider = pair.getKey();
final List<String> requirements = pair.getValue();
if (requirements != null && requirements.size() > 0) {
destination.append(NEW_LINE);
destination.append("Unmet requirements for the '").append(provider.getClassName()).append("' provider:").append(NEW_LINE);
for (final String requirement : requirements) {
destination.append(" - ").append(requirement).append(NEW_LINE);
}
}
}
}

static void appendProperties(final Properties properties, final StringBuilder destination) {
Expand Down
34 changes: 34 additions & 0 deletions ...2-useragent-core/src/main/java/com/microsoft/alm/oauth2/useragent/utils/StringHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root.

package com.microsoft.alm.oauth2.useragent.utils;

public class StringHelper {

public static boolean equal(final String s1, final String s2) {
if (s1 == null) {
return s2 == null;
}
return s1.equals(s2);
}

public static String join(final String separator, final String[] value) {
if (value == null)
throw new IllegalArgumentException("value is null");

// "If separator is null, an empty string (String.Empty) is used instead."
final String sep = separator == null ? "" : separator;

final StringBuilder result = new StringBuilder();

if (value.length > 0) {
result.append(value[0] == null ? "" : value[0]);
for (int i = 1; i < value.length; i++) {
result.append(sep);
result.append(value[i] == null ? "" : value[i]);
}
}

return result.toString();
}
}
Loading