Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/main/java/org/kohsuke/github/GHEventInfo.java
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ public GHOrganization getOrganization() throws IOException {
* if payload cannot be parsed
*/
public <T extends GHEventPayload> T getPayload(Class<T> type) throws IOException {
T v = GitHubClient.MAPPER.readValue(payload.traverse(), type);
T v = GitHubClient.getMappingObjectReader().readValue(payload.traverse(), type);
v.wrapUp(root);
return v;
}
Expand Down
14 changes: 14 additions & 0 deletions src/main/java/org/kohsuke/github/GHObject.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.kohsuke.github;

import com.fasterxml.jackson.annotation.JacksonInject;
import com.infradna.tool.bridge_method_injector.WithBridgeMethods;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import org.apache.commons.lang3.builder.ReflectionToStringBuilder;
Expand Down Expand Up @@ -33,6 +34,19 @@ public abstract class GHObject {
GHObject() {
}

/**
* Called by Jackson
*
* @param responseInfo
* the {@link GitHubResponse.ResponseInfo} to get headers from.
*/
@JacksonInject
protected void setResponseHeaderFields(@CheckForNull GitHubResponse.ResponseInfo responseInfo) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is what we do instead of the special case.

if (responseInfo != null) {
responseHeaderFields = responseInfo.headers();
}
}

/**
* Returns the HTTP response headers given along with the state of this object.
*
Expand Down
35 changes: 20 additions & 15 deletions src/main/java/org/kohsuke/github/GHRateLimit.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.kohsuke.github;

import com.fasterxml.jackson.annotation.JacksonInject;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
Expand Down Expand Up @@ -289,14 +290,11 @@ public static class Record {

/**
* The time at which the rate limit will reset. This value is calculated based on
* {@link #getResetEpochSeconds()} by calling {@link #recalculateResetDate}. If the clock on the local machine
* not synchronized with the server clock, this time value will be adjusted to match the local machine's clock.
* <p>
* Recalculated by calling {@link #recalculateResetDate}.
* </p>
* {@link #getResetEpochSeconds()} by calling {@link #calculateResetDate}. If the clock on the local machine not
* synchronized with the server clock, this time value will be adjusted to match the local machine's clock.
*/
@Nonnull
private Date resetDate;
private final Date resetDate;

/**
* Instantiates a new Record.
Expand All @@ -308,31 +306,37 @@ public static class Record {
* @param resetEpochSeconds
* the reset epoch seconds
*/
@JsonCreator
public Record(@JsonProperty(value = "limit", required = true) int limit,
@JsonProperty(value = "remaining", required = true) int remaining,
@JsonProperty(value = "reset", required = true) long resetEpochSeconds) {
this(limit, remaining, resetEpochSeconds, null);
}

/**
* Instantiates a new Record.
* Instantiates a new Record. Called by Jackson data binding or during header parsing.
*
* @param limit
* the limit
* @param remaining
* the remaining
* @param resetEpochSeconds
* the reset epoch seconds
* @param updatedAt
* the updated at
* @param responseInfo
* the response info
*/
@SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD", justification = "Deprecated")
public Record(int limit, int remaining, long resetEpochSeconds, @CheckForNull String updatedAt) {
@JsonCreator
Record(@JsonProperty(value = "limit", required = true) int limit,
@JsonProperty(value = "remaining", required = true) int remaining,
@JsonProperty(value = "reset", required = true) long resetEpochSeconds,
@JacksonInject @CheckForNull GitHubResponse.ResponseInfo responseInfo) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Also, since we now have the the responseInfo injected during binding, we can make Record fully immutable. We don't have "recalculate" after creation.

this.limit = limit;
this.remaining = remaining;
this.resetEpochSeconds = resetEpochSeconds;
this.resetDate = recalculateResetDate(updatedAt);
String updatedAt = null;
if (responseInfo != null) {
updatedAt = responseInfo.headerField("Date");
}
this.resetDate = calculateResetDate(updatedAt);
}

/**
Expand Down Expand Up @@ -362,7 +366,8 @@ public Record(int limit, int remaining, long resetEpochSeconds, @CheckForNull St
* a string date in RFC 1123
* @return reset date based on the passed date
*/
Date recalculateResetDate(@CheckForNull String updatedAt) {
@Nonnull
private Date calculateResetDate(@CheckForNull String updatedAt) {
long updatedAtEpochSeconds = createdAtEpochSeconds;
if (!StringUtils.isBlank(updatedAt)) {
try {
Expand All @@ -379,7 +384,7 @@ Date recalculateResetDate(@CheckForNull String updatedAt) {
// This may seem odd but it results in an accurate or slightly pessimistic reset date
// based on system time rather than assuming the system time synchronized with the server
long calculatedSecondsUntilReset = resetEpochSeconds - updatedAtEpochSeconds;
return resetDate = new Date((createdAtEpochSeconds + calculatedSecondsUntilReset) * 1000);
return new Date((createdAtEpochSeconds + calculatedSecondsUntilReset) * 1000);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/kohsuke/github/GitHub.java
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,7 @@ public GHGistBuilder createGist() {
* the io exception
*/
public <T extends GHEventPayload> T parseEventPayload(Reader r, Class<T> type) throws IOException {
T t = GitHubClient.MAPPER.readValue(r, type);
T t = GitHubClient.getMappingObjectReader().forType(type).readValue(r);
t.wrapUp(this);
return t;
}
Expand Down
73 changes: 46 additions & 27 deletions src/main/java/org/kohsuke/github/GitHubClient.java
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
package org.kohsuke.github;

import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.InjectableValues;
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.ObjectReader;
import com.fasterxml.jackson.databind.ObjectWriter;
import com.fasterxml.jackson.databind.PropertyNamingStrategy;
import com.fasterxml.jackson.databind.introspect.VisibilityChecker;
import org.apache.commons.lang3.StringUtils;
Expand Down Expand Up @@ -73,7 +76,7 @@ abstract class GitHubClient {

private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName());

static final ObjectMapper MAPPER = new ObjectMapper();
private static final ObjectMapper MAPPER = new ObjectMapper();
static final String GITHUB_URL = "https://api.github.com";

private static final String[] TIME_FORMATS = { "yyyy/MM/dd HH:mm:ss ZZZZ", "yyyy-MM-dd'T'HH:mm:ss'Z'",
Expand Down Expand Up @@ -399,7 +402,6 @@ private static <T> GitHubResponse<T> createResponse(@Nonnull GitHubResponse.Resp
// Maybe throw an exception instead?
} else if (handler != null) {
body = handler.apply(responseInfo);
setResponseHeaders(responseInfo, body);
}
return new GitHubResponse<>(responseInfo, body);
}
Expand Down Expand Up @@ -443,30 +445,6 @@ private static IOException interpretApiError(IOException e,
return e;
}

/**
* Sets the response headers on objects that need it. Ideally this would be handled by the objects themselves, but
* currently they do not have access to {@link GitHubResponse.ResponseInfo} after the
*
* @param responseInfo
* the response info
* @param readValue
* the object to consider adding headers to.
* @param <T>
* type of the object
*/
private static <T> void setResponseHeaders(GitHubResponse.ResponseInfo responseInfo, T readValue) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the special case code that we had before.

if (readValue instanceof GHObject[]) {
for (GHObject ghObject : (GHObject[]) readValue) {
ghObject.responseHeaderFields = responseInfo.headers();
}
} else if (readValue instanceof GHObject) {
((GHObject) readValue).responseHeaderFields = responseInfo.headers();
} else if (readValue instanceof JsonRateLimit) {
// if we're getting a GHRateLimit it needs the server date
((JsonRateLimit) readValue).resources.getCore().recalculateResetDate(responseInfo.headerField("Date"));
}
}

protected static boolean isRateLimitResponse(@Nonnull GitHubResponse.ResponseInfo responseInfo) {
return responseInfo.statusCode() == HttpURLConnection.HTTP_FORBIDDEN
&& "0".equals(responseInfo.headerField("X-RateLimit-Remaining"));
Expand Down Expand Up @@ -567,7 +545,7 @@ private void noteRateLimit(@Nonnull GitHubResponse.ResponseInfo responseInfo) {
return;
}

GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, responseInfo.headerField("Date"));
GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, responseInfo);

updateCoreRateLimit(observed);
}
Expand Down Expand Up @@ -714,4 +692,45 @@ static String printDate(Date dt) {
df.setTimeZone(TimeZone.getTimeZone("GMT"));
return df.format(dt);
}

/**
* Gets an {@link ObjectWriter}.
*
* @return an {@link ObjectWriter} instance that can be further configured.
*/
@Nonnull
static ObjectWriter getMappingObjectWriter() {
return MAPPER.writer();
}

/**
* Helper for {@link #getMappingObjectReader(GitHubResponse.ResponseInfo)}
*
* @return an {@link ObjectReader} instance that can be further configured.
*/
@Nonnull
static ObjectReader getMappingObjectReader() {
return getMappingObjectReader(null);
}

/**
* Gets an {@link ObjectReader}.
*
* Members of {@link InjectableValues} must be present even if {@code null}, otherwise classes expecting those
* values will fail to read. This differs from regular JSONProperties which provide defaults instead of failing.
*
* Having one spot to create readers and having it take all injectable values is not a great long term solution but
* it is sufficient for this first cut.
*
* @param responseInfo
* the {@link GitHubResponse.ResponseInfo} to inject for this reader.
* @return an {@link ObjectReader} instance that can be further configured.
*/
@Nonnull
static ObjectReader getMappingObjectReader(@CheckForNull GitHubResponse.ResponseInfo responseInfo) {
InjectableValues.Std inject = new InjectableValues.Std();
inject.addValue(GitHubResponse.ResponseInfo.class, responseInfo);

return MAPPER.reader(inject);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ private static void buildRequest(GitHubRequest request, HttpURLConnection connec
for (GitHubRequest.Entry e : request.args()) {
json.put(e.key, e.value);
}
MAPPER.writeValue(connection.getOutputStream(), json);
getMappingObjectWriter().writeValue(connection.getOutputStream(), json);
}
}
}
Expand Down Expand Up @@ -235,4 +235,5 @@ private InputStream wrapStream(InputStream stream) throws IOException {
private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName());

}

}
8 changes: 6 additions & 2 deletions src/main/java/org/kohsuke/github/GitHubResponse.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.kohsuke.github;

import com.fasterxml.jackson.databind.InjectableValues;
import com.fasterxml.jackson.databind.JsonMappingException;
import org.apache.commons.io.IOUtils;

Expand Down Expand Up @@ -77,7 +78,10 @@ static <T> T parseBody(ResponseInfo responseInfo, Class<T> type) throws IOExcept

String data = responseInfo.getBodyAsString();
try {
return GitHubClient.MAPPER.readValue(data, type);
InjectableValues.Std inject = new InjectableValues.Std();
inject.addValue(ResponseInfo.class, responseInfo);

return GitHubClient.getMappingObjectReader(responseInfo).forType(type).readValue(data);
} catch (JsonMappingException e) {
String message = "Failed to deserialize " + data;
throw new IOException(message, e);
Expand All @@ -102,7 +106,7 @@ static <T> T parseBody(ResponseInfo responseInfo, T instance) throws IOException

String data = responseInfo.getBodyAsString();
try {
return GitHubClient.MAPPER.readerForUpdating(instance).<T>readValue(data);
return GitHubClient.getMappingObjectReader(responseInfo).withValueToUpdate(instance).readValue(data);
} catch (JsonMappingException e) {
String message = "Failed to deserialize " + data;
throw new IOException(message, e);
Expand Down
4 changes: 4 additions & 0 deletions src/test/java/org/kohsuke/github/GHObjectTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,9 @@ public void test_toString() throws Exception {
assertThat(org.toString(),
containsString(
"login=github-api-test-org,location=<null>,blog=<null>,email=<null>,name=<null>,company=<null>,type=Organization,followers=0,following=0"));

// getResponseHeaderFields is deprecated but we should not break it.
assertThat(org.getResponseHeaderFields(), notNullValue());
assertThat(org.getResponseHeaderFields().get("Cache-Control").get(0), is("private, max-age=60, s-maxage=60"));
}
}