Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Encode special characters in ADLS Gen2 URI #3937

Merged
merged 3 commits into from
Jan 16, 2020
Merged

Encode special characters in ADLS Gen2 URI #3937

merged 3 commits into from
Jan 16, 2020

Conversation

t-rufang
Copy link
Contributor

@t-rufang t-rufang commented Jan 15, 2020 •
edited
Loading

Changed

  • Multiple paths in referenced jars and referenced files should be split by space rather than semicolon

Fixed

Feature Request

  • Currently we only encode URI for user selected jars & files on VFS for Gen2. We should also encode URI for user input ADLS Gen2 URI, ADLS Gen1 URI and Azure Blob URI. Impacted user input text fields contains user input referenced jars/files, Gen2 root path in linked Gen2 storage card, Gen2 root path in linked ESP Gen2 storage card, Gen1 path in linked Gen1 storage card.

@t-rufang t-rufang requested a review from wezhang January 15, 2020 11:57
wezhang
wezhang reviewed Jan 16, 2020
View reviewed changes
Copy link
Member

@wezhang wezhang left a comment

Choose a reason for hiding this comment

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

Thanks Rui! It's a deep hidden bug.

...olkit-for-intellij/src/com/microsoft/azure/hdinsight/spark/ui/SparkSubmissionContentPanel.kt
@@ -298,10 +297,10 @@ open class SparkSubmissionContentPanel(private val myProject: Project, val type:
}

private val refJarsPrompt: JLabel = JLabel("Referenced Jars(spark.jars)").apply {
toolTipText = "Files to be placed on the java classpath; The path needs to be an Azure Blob Storage Path (path started with wasb://); Multiple paths should be split by semicolon (;)"
toolTipText = "Files to be placed on the java classpath. Multiple paths should be split by space."
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we also need to update documents.

Utils/hdinsight-node-common/src/com/microsoft/azure/hdinsight/common/AbfsUri.java Outdated
}

@Override
AzureStorageUri normalizeWithSlashEnding() {
return AbfsUri.parse(UriUtil.normalizeWithSlashEnding(getUri()).toString());
AzureStorageUri parseUri(String encodedUri) {
Copy link
Member

@wezhang wezhang Jan 16, 2020 •
edited by t-rufang
Loading

Choose a reason for hiding this comment

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

The type might have to be aligned with name.

Suggested change
AzureStorageUri parseUri(String encodedUri) {
AzureStorageUri parseUri(URI encodedUri) {
``` #Resolved

Utils/hdinsight-node-common/src/com/microsoft/azure/hdinsight/common/AzureStorageUri.java
@@ -47,7 +49,7 @@ public URI getRawUri() {
}

/**
* Get URI with specified scheme, not limited to HTTP or HTTPS
* Get URI with specified scheme except HTTP and HTTPS
Copy link
Member

@wezhang wezhang Jan 16, 2020 •
edited by t-rufang
Loading

Choose a reason for hiding this comment

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

Thanks! This is just the right expression. #Resolved

Utils/hdinsight-node-common/src/com/microsoft/azure/hdinsight/common/AbfsUri.java Outdated
public static final Pattern ABFS_URI_PATTERN = Pattern.compile(AdlsGen2PathPattern, Pattern.CASE_INSENSITIVE);
public static final Pattern HTTP_URI_PATTERN = Pattern.compile(AdlsGen2RestfulPathPattern, Pattern.CASE_INSENSITIVE);

private final LaterInit<String> fileSystem = new LaterInit<>();
private final LaterInit<String> accountName = new LaterInit<>();
private final LaterInit<String> relativePath = new LaterInit<>();
private final LaterInit<String> path = new LaterInit<>();
Copy link
Member

@wezhang wezhang Jan 16, 2020 •
edited by t-rufang
Loading

Choose a reason for hiding this comment

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

Looks like path can be a member of base class. #Resolved

Utils/hdinsight-node-common/src/com/microsoft/azure/hdinsight/common/AbfsUri.java Outdated
@@ -57,42 +57,43 @@ public String getAccountName() {

@Override
public String getPath() {
return relativePath.get();
return URI.create(path.get()).getPath();
Copy link
Member

@wezhang wezhang Jan 16, 2020 •
edited by t-rufang
Loading

Choose a reason for hiding this comment

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

Should we store the path as URI? #Resolved

Utils/hdinsight-node-common/src/com/microsoft/azure/hdinsight/common/AzureStorageUri.java Outdated
* @param rawPath un-encoded raw path in the Azure Storage URI
* @return encoded path in the Azure Storage URI
*/
public static String encodePath(String rawPath) {
Copy link
Member

Choose a reason for hiding this comment

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

I suppose we don't need this method any more. Refer to the code .resolve("./" + target)

Copy link
Member

@wezhang wezhang Jan 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Let's try this:

public static String encodeWithNormalizePath(String rawPath) {
    String encodedPath = URI.create("/").relativize(new URIBuilder().setPath("/" + rawPath).build());
    return rawPath.startsWith("/") ? "/" + encodedPath : encodedPath;
}

@t-rufang t-rufang requested a review from wezhang January 16, 2020 07:27
wezhang
wezhang approved these changes Jan 16, 2020
View reviewed changes
Copy link
Member

@wezhang wezhang left a comment

Choose a reason for hiding this comment

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

LGTM! Shipit!

@t-rufang t-rufang merged commit 3c1a482 into microsoft:develop Jan 16, 2020
wezhang added a commit to wezhang/azure-tools-for-java that referenced this pull request Jan 19, 2020
@wezhang
Merge commit '3c1a482d3f1e21fe5ac08d5dcc923c615b543b27' into develop.…
80ede83 
…next

Encode special characters in ADLS Gen2 URI (microsoft#3937)
andxu pushed a commit to andxu/azure-tools-for-java that referenced this pull request Mar 24, 2020
@t-rufang
Encode special characters in ADLS Gen2 URI (microsoft#3937)
bd49ccc 
* Encode special characters in ADLS Gen2 URI
andxu pushed a commit to andxu/azure-tools-for-java that referenced this pull request Mar 24, 2020
@wezhang
Merge commit '3c1a482d3f1e21fe5ac08d5dcc923c615b543b27' into develop.…
7f85084 
…next

Encode special characters in ADLS Gen2 URI (microsoft#3937)
@t-rufang t-rufang deleted the issue_3899 branch April 10, 2020 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wezhang wezhang wezhang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@t-rufang @wezhang