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

Avoid caching when getting a raw file from raw.githubusercontent.com #376

Merged
merged 1 commit into from
Nov 14, 2022
Merged

Avoid caching when getting a raw file from raw.githubusercontent.com #376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
91 changes: 81 additions & 10 deletions ...ctory-github/src/main/java/org/eclipse/che/api/factory/server/github/GithubApiClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.io.InputStreamReader;
import java.io.UncheckedIOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
Expand All @@ -52,6 +53,10 @@ public class GithubApiClient {
/** GitHub endpoint URL. */
public static final String GITHUB_SAAS_ENDPOINT = "https://github.com";

public static final String GITHUB_SAAS_ENDPOINT_API = "https://api.github.com";

public static final String GITHUB_SAAS_ENDPOINT_RAW = "https://raw.githubusercontent.com";

/** GitHub HTTP header containing OAuth scopes. */
public static final String GITHUB_OAUTH_SCOPES_HEADER = "X-OAuth-Scopes";

Expand All @@ -68,7 +73,7 @@ public GithubApiClient(@Nullable String serverUrl) {
this.apiServerUrl =
URI.create(
isNullOrEmpty(trimmedServerUrl) || trimmedServerUrl.equals(GITHUB_SAAS_ENDPOINT)
? "https://api.github.com/"
? GITHUB_SAAS_ENDPOINT_API + "/"
: trimmedServerUrl + "/api/v3/");
this.scmServerUrl =
URI.create(isNullOrEmpty(trimmedServerUrl) ? GITHUB_SAAS_ENDPOINT : trimmedServerUrl);
Expand Down Expand Up @@ -113,6 +118,16 @@ public GithubUser getUser(String authenticationToken)
});
}

/**
* Fetches and returns a pull request.
*
* @param id pull request ID
* @param username user name
* @param repoName repository name
* @param authenticationToken oauth access token, can be NULL as the GitHub can handle some
* requests without authentication
* @return pull request
*/
public GithubPullRequest getPullRequest(
String id, String username, String repoName, String authenticationToken)
throws ScmItemNotFoundException, ScmCommunicationException, ScmBadRequestException {
Expand All @@ -132,6 +147,47 @@ public GithubPullRequest getPullRequest(
});
}

/**
* Returns the latest commit of the branch.
*
* <p>GitHub REST API documentation: https://docs.github.com/en/rest/commits/commits
*
* @param user user or organization name
* @param repository repository name
* @param branch required branch
* @param authenticationToken OAuth access token, can be NULL as the GitHub can handle some
* requests without authentication
* @return the latest commit of the branch
*/
public GithubCommit getLatestCommit(
String user, String repository, String branch, @Nullable String authenticationToken)
throws ScmBadRequestException, ScmItemNotFoundException, ScmCommunicationException,
URISyntaxException {

final URI uri = apiServerUrl.resolve(String.format("./repos/%s/%s/commits", user, repository));

final URI requestURI =
new URI(
uri.getScheme(),
uri.getAuthority(),
uri.getPath(),
String.format("sha=%s&page=1&per_page=1", branch),
null);
HttpRequest request = buildGithubApiRequest(requestURI, authenticationToken);
LOG.trace("executeRequest={}", request);

return executeRequest(
httpClient,
request,
response -> {
try {
return OBJECT_MAPPER.readValue(response.body(), GithubCommit[].class)[0];
} catch (IOException e) {
throw new UncheckedIOException(e);
}
});
}

/**
* Returns the scopes of the OAuth token.
*
Expand Down Expand Up @@ -162,15 +218,30 @@ public String[] getTokenScopes(String authenticationToken)
});
}

private HttpRequest buildGithubApiRequest(URI uri, String authenticationToken) {
return HttpRequest.newBuilder(uri)
.headers(
"Authorization",
"token " + authenticationToken,
"Accept",
"application/vnd.github.v3+json")
.timeout(DEFAULT_HTTP_TIMEOUT)
.build();
/**
* Builds and returns HttpRequest to acces the GitHub API.
*
* @param uri request uri
* @param authenticationToken authentication token, can be NULL as the GitHub can handle some
* requests without authentication
* @return HttpRequest object
*/
private HttpRequest buildGithubApiRequest(URI uri, @Nullable String authenticationToken) {
if (isNullOrEmpty(authenticationToken)) {
return HttpRequest.newBuilder(uri)
.headers("Accept", "application/vnd.github.v3+json")
.timeout(DEFAULT_HTTP_TIMEOUT)
.build();
} else {
return HttpRequest.newBuilder(uri)
.headers(
"Authorization",
"token " + authenticationToken,
"Accept",
"application/vnd.github.v3+json")
.timeout(DEFAULT_HTTP_TIMEOUT)
.build();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a code duplication, why not to define the headers array out of the return statement?

String[] headers = {"Accept", "application/vnd.github.v3+json"};
if (!isNullOrEmpty(authenticationToken)) {
  headers[2] = "Authorization";
  headers[3] = "token " + authenticationToken;
}
return HttpRequest.newBuilder(uri)
        .headers(headers)
        .timeout(DEFAULT_HTTP_TIMEOUT)
        .build();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it's a duplication.
There is only one if statement, which makes the code easy to read.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a duplication because there are several identical code lines:

  private HttpRequest buildGithubApiRequest(URI uri, @Nullable String authenticationToken) {
    if (isNullOrEmpty(authenticationToken)) {
>     return HttpRequest.newBuilder(uri)
          .headers("Accept", "application/vnd.github.v3+json")
>         .timeout(DEFAULT_HTTP_TIMEOUT)
>         .build();
    } else {
>     return HttpRequest.newBuilder(uri)
          .headers(
              "Authorization",
              "token " + authenticationToken,
              "Accept",
              "application/vnd.github.v3+json")
>         .timeout(DEFAULT_HTTP_TIMEOUT)
>         .build();
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a question of taste and it should not block merging of this pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't block the merging, I just express my point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or a little bit simpler with wrapping to list:

private HttpRequest buildGithubApiRequest(URI uri, String authenticationToken) {
  List<String> headers = Arrays.asList("Accept", "application/vnd.github.v3+json");
  if (!isNullOrEmpty(authenticationToken)) {
    headers.addAll(Arrays.asList("Authorization", "token " + authenticationToken));
  }
  return HttpRequest.newBuilder(uri)
      .headers(headers.toArray(String[]::new))
      .timeout(DEFAULT_HTTP_TIMEOUT)
      .build();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks much better. Let's go with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It fails with java.lang.UnsupportedOperationException on

headers.addAll(Arrays.asList("Authorization", "token " + authenticationToken));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a code duplication, why not to define the headers array out of the return statement?

String[] headers = {"Accept", "application/vnd.github.v3+json"};
if (!isNullOrEmpty(authenticationToken)) {
  headers[2] = "Authorization";

Throws ArrayIndexOutOfBoundsException

java.lang.ArrayIndexOutOfBoundsException: Index 2 out of bounds for length 2

headers[3] = "token " + authenticationToken;
}
return HttpRequest.newBuilder(uri)
.headers(headers)
.timeout(DEFAULT_HTTP_TIMEOUT)
.build();

Copy link
Contributor

Choose a reason for hiding this comment

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

Throws ArrayIndexOutOfBoundsException

I haven't tested it but I think wrapping to a list should work:

List<String> headers = Arrays.asList("Accept", "application/vnd.github.v3+json");
if (!isNullOrEmpty(authenticationToken)) {
  headers.addAll(Arrays.asList("Authorization", "token " + authenticationToken));
}

}
}

private <T> T executeRequest(
Expand Down
49 changes: 49 additions & 0 deletions ...-factory-github/src/main/java/org/eclipse/che/api/factory/server/github/GithubCommit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Copyright (c) 2012-2022 Red Hat, Inc.
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*/

package org.eclipse.che.api.factory.server.github;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;

@JsonIgnoreProperties(ignoreUnknown = true)
public class GithubCommit {

private String sha;

private String url;

public String getSha() {
return sha;
}

public void setSha(String sha) {
this.sha = sha;
}

public GithubCommit withSha(String sha) {
this.sha = sha;
return this;
}

public String getUrl() {
return url;
}

public void setUrl(String url) {
this.url = url;
}

public GithubCommit withUrl(String url) {
this.url = url;
return this;
}
}
19 changes: 19 additions & 0 deletions .../main/java/org/eclipse/che/api/factory/server/github/GithubFactoryParametersResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import jakarta.validation.constraints.NotNull;
import java.util.Map;
import javax.inject.Inject;
import javax.inject.Named;
import javax.inject.Singleton;
import org.eclipse.che.api.core.ApiException;
import org.eclipse.che.api.core.BadRequestException;
Expand All @@ -35,6 +36,7 @@
import org.eclipse.che.api.workspace.server.devfile.URLFetcher;
import org.eclipse.che.api.workspace.shared.dto.ProjectConfigDto;
import org.eclipse.che.api.workspace.shared.dto.devfile.ProjectDto;
import org.eclipse.che.commons.annotation.Nullable;

/**
* Provides Factory Parameters resolver for github repositories.
Expand All @@ -57,6 +59,23 @@ public class GithubFactoryParametersResolver extends DefaultFactoryParameterReso

@Inject
public GithubFactoryParametersResolver(
GithubURLParser githubUrlParser,
URLFetcher urlFetcher,
GithubSourceStorageBuilder githubSourceStorageBuilder,
URLFactoryBuilder urlFactoryBuilder,
ProjectConfigDtoMerger projectConfigDtoMerger,
PersonalAccessTokenManager personalAccessTokenManager,
@Nullable @Named("che.integration.github.oauth_endpoint") String oauthEndpoint) {
this(
githubUrlParser,
urlFetcher,
githubSourceStorageBuilder,
urlFactoryBuilder,
projectConfigDtoMerger,
personalAccessTokenManager);
}

GithubFactoryParametersResolver(
GithubURLParser githubUrlParser,
URLFetcher urlFetcher,
GithubSourceStorageBuilder githubSourceStorageBuilder,
Expand Down
30 changes: 30 additions & 0 deletions ...ory-github/src/main/java/org/eclipse/che/api/factory/server/github/GithubPullRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ public String getName() {
public void setName(String name) {
this.name = name;
}

public GithubRepo withName(String name) {
this.name = name;
return this;
}
}

@JsonIgnoreProperties(ignoreUnknown = true)
Expand All @@ -40,6 +45,11 @@ public void setRef(String ref) {
this.ref = ref;
}

public GithubHead withRef(String ref) {
this.ref = ref;
return this;
}

public GithubUser getUser() {
return user;
}
Expand All @@ -48,13 +58,23 @@ public void setUser(GithubUser user) {
this.user = user;
}

public GithubHead withUser(GithubUser user) {
this.user = user;
return this;
}

public GithubRepo getRepo() {
return repo;
}

public void setRepo(GithubRepo repo) {
this.repo = repo;
}

public GithubHead withRepo(GithubRepo repo) {
this.repo = repo;
return this;
}
}

@JsonIgnoreProperties(ignoreUnknown = true)
Expand All @@ -70,11 +90,21 @@ public void setState(String state) {
this.state = state;
}

public GithubPullRequest withState(String state) {
this.state = state;
return this;
}

public GithubHead getHead() {
return head;
}

public void setHead(GithubHead head) {
this.head = head;
}

public GithubPullRequest withHead(GithubHead head) {
this.head = head;
return this;
}
}
Loading