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.

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

Remove java.util.Date #2373

Merged
merged 1 commit into from
Mar 15, 2024
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
11 changes: 9 additions & 2 deletions config/presubmits.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,17 +173,24 @@ def fails(self, file):
):
"JavaScript files should not include console logging.",
PresubmitCheck(
r"org\.testcontainers\.shaded\.",
r".*org\.testcontainers\.shaded.*",
"java",
{"/node_modules/"},
):
"Do not use shaded dependencies from testcontainers.",
PresubmitCheck(
r"com\.google\.common\.truth\.Truth8",
r".*com\.google\.common\.truth\.Truth8.*",
"java",
{"/node_modules/"},
):
"Truth8 is deprecated. Use Truth instead.",
PresubmitCheck(
r".*java\.util\.Date.*",
"java",
{"/node_modules/", "JpaTransactionManagerImpl.java"},
):
"Do not use java.util.Date. Use classes in java.time package instead.",

}

# Note that this regex only works for one kind of Flyway file. If we want to
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
import java.math.BigDecimal;
import java.time.Duration;
import java.util.Collection;
import java.util.Date;
import java.util.Map;
import java.util.ServiceLoader;
import org.apache.commons.codec.binary.Base64;
Expand Down Expand Up @@ -195,10 +194,7 @@ public AccessToken refreshAccessToken() throws IOException {
GenericData responseData = response.parseAs(GenericData.class);
String accessToken = validateString(responseData, "access_token", PARSE_ERROR_PREFIX);
int expiresInSeconds = validateInt32(responseData, "expires_in", PARSE_ERROR_PREFIX);
long expiresAtMilliseconds = clock.nowUtc().getMillis() + expiresInSeconds * 1000L;
@SuppressWarnings("JavaUtilDate")
AccessToken token = new AccessToken(accessToken, new Date(expiresAtMilliseconds));
return token;
return new AccessToken(accessToken, clock.nowUtc().plusSeconds(expiresInSeconds).toDate());
}

String createAssertion(JsonFactory jsonFactory, long currentTime) throws IOException {
Expand Down Expand Up @@ -257,8 +253,7 @@ static int validateInt32(Map<String, Object> map, String key, String errorPrefix
if (value == null) {
throw new IOException(String.format(VALUE_NOT_FOUND_MESSAGE, errorPrefix, key));
}
if (value instanceof BigDecimal) {
BigDecimal bigDecimalValue = (BigDecimal) value;
if (value instanceof BigDecimal bigDecimalValue) {
return bigDecimalValue.intValueExact();
}
if (!(value instanceof Integer)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import java.security.cert.X509Certificate;
import java.security.interfaces.ECPublicKey;
import java.security.interfaces.RSAPublicKey;
import java.util.Date;
import java.util.stream.Collectors;
import javax.inject.Inject;
import org.bouncycastle.jcajce.provider.asymmetric.util.EC5Util;
Expand Down Expand Up @@ -106,12 +105,9 @@ private static boolean checkCurveName(PublicKey key, ImmutableSet<String> allowe
// These 2 different instances of PublicKey need to be handled separately since their OIDs are
// encoded differently. More details on this can be found at
// https://stackoverflow.com/questions/49895713/how-to-find-the-matching-curve-name-from-an-ecpublickey.
if (key instanceof ECPublicKey) {
ECPublicKey ecKey = (ECPublicKey) key;
if (key instanceof ECPublicKey ecKey) {
params = EC5Util.convertSpec(ecKey.getParams());
} else if (key instanceof org.bouncycastle.jce.interfaces.ECPublicKey) {
org.bouncycastle.jce.interfaces.ECPublicKey ecKey =
(org.bouncycastle.jce.interfaces.ECPublicKey) key;
} else if (key instanceof org.bouncycastle.jce.interfaces.ECPublicKey ecKey) {
params = ecKey.getParameters();
} else {
throw new IllegalArgumentException("Unrecognized instance of PublicKey.");
Expand Down Expand Up @@ -148,7 +144,7 @@ private void handleCertViolations(ImmutableSet<CertificateViolation> violations)
if (!violations.isEmpty()) {
String displayMessages =
violations.stream()
.map(violation -> getViolationDisplayMessage(violation))
.map(this::getViolationDisplayMessage)
.collect(Collectors.joining("\n"));
throw new InsecureCertificateException(violations, displayMessages);
}
Expand All @@ -162,7 +158,7 @@ public ImmutableSet<CertificateViolation> checkCertificate(X509Certificate certi
ImmutableSet.Builder<CertificateViolation> violations = new ImmutableSet.Builder<>();

// Check if currently in validity period
Date now = clock.nowUtc().toDate();
DateTime now = clock.nowUtc();
Copy link
Member

Choose a reason for hiding this comment

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

This is a functionality change? Are you sure this is even correct?

What's the motivation for removing Date everywhere anyway? Sometimes you do legitimately need to process just a date, not a date and an attached time.

if (DateTimeComparator.getInstance().compare(certificate.getNotAfter(), now) < 0) {
violations.add(CertificateViolation.EXPIRED);
} else if (DateTimeComparator.getInstance().compare(certificate.getNotBefore(), now) > 0) {
Expand Down Expand Up @@ -231,13 +227,13 @@ public boolean shouldReceiveExpiringNotification(
DateTime lastExpiringNotificationSentDate, String certificateStr) {
X509Certificate certificate = getCertificate(certificateStr);
DateTime now = clock.nowUtc();
// expiration date is one day after lastValidDate
// the expiration date is one day after lastValidDate
DateTime lastValidDate = new DateTime(certificate.getNotAfter());
if (lastValidDate.isBefore(now)) {
return false;
}
/*
* Client should receive a notification if :
* Client should receive a notification if:
* 1) client has never received notification (lastExpiringNotificationSentDate is initially
* set to START_OF_TIME) and the certificate has entered the expiring period, OR
* 2) client has received notification but the interval between now and
Expand All @@ -254,29 +250,21 @@ private String getViolationDisplayMessage(CertificateViolation certificateViolat
// Yes, we'd rather do this as an instance method on the CertificateViolation enum itself, but
// we can't because we need access to configuration (injected as instance variables) which you
// can't get in a static enum context.
switch (certificateViolation) {
case EXPIRED:
return "Certificate is expired.";
case NOT_YET_VALID:
return "Certificate start date is in the future.";
case ALGORITHM_CONSTRAINED:
return "Certificate key algorithm must be RSA or ECDSA.";
case RSA_KEY_LENGTH_TOO_SHORT:
return String.format(
"RSA key length is too short; the minimum allowed length is %d bits.",
this.minimumRsaKeyLength);
case VALIDITY_LENGTH_TOO_LONG:
return String.format(
"Certificate validity period is too long; it must be less than or equal to %d days.",
this.maxValidityLengthSchedule.lastEntry().getValue());
case INVALID_ECDSA_CURVE:
return String.format(
"The ECDSA key must use one of these algorithms: %s", allowedEcdsaCurves);
default:
throw new IllegalArgumentException(
String.format(
"Unknown CertificateViolation enum value: %s", certificateViolation.name()));
}
return switch (certificateViolation) {
case EXPIRED -> "Certificate is expired.";
case NOT_YET_VALID -> "Certificate start date is in the future.";
case ALGORITHM_CONSTRAINED -> "Certificate key algorithm must be RSA or ECDSA.";
case RSA_KEY_LENGTH_TOO_SHORT ->
String.format(
"RSA key length is too short; the minimum allowed length is %d bits.",
this.minimumRsaKeyLength);
case VALIDITY_LENGTH_TOO_LONG ->
String.format(
"Certificate validity period is too long; it must be less than or equal to %d days.",
this.maxValidityLengthSchedule.lastEntry().getValue());
case INVALID_ECDSA_CURVE ->
String.format("The ECDSA key must use one of these algorithms: %s", allowedEcdsaCurves);
};
}

/**
Expand All @@ -295,7 +283,7 @@ public enum CertificateViolation {
* Gets a suitable end-user-facing display message for this particular certificate violation.
*
* <p>Note that the {@link CertificateChecker} instance must be passed in because it contains
* configuration values (e.g. minimum RSA key length) that go into the error message text.
* configuration values (e.g., minimum RSA key length) that go into the error message text.
*/
public String getDisplayMessage(CertificateChecker certificateChecker) {
return certificateChecker.getViolationDisplayMessage(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ private static ImmutableMap<TmchCaMode, X509Certificate> loadRootCertificates()
*/
public void verify(X509Certificate cert) throws GeneralSecurityException {
synchronized (TmchCertificateAuthority.class) {
X509Utils.verifyCertificate(getAndValidateRoot(), getCrl(), cert, clock.nowUtc().toDate());
X509Utils.verifyCertificate(getAndValidateRoot(), getCrl(), cert, clock.nowUtc());
}
}

Expand All @@ -151,7 +151,7 @@ public void updateCrl(String asciiCrl, String url) throws GeneralSecurityExcepti
} catch (Exception e) {
logger.atWarning().withCause(e).log("Old CRL is invalid, ignored during CRL update.");
}
X509Utils.verifyCrl(getAndValidateRoot(), oldCrl, newCrl, clock.nowUtc().toDate());
X509Utils.verifyCrl(getAndValidateRoot(), oldCrl, newCrl, clock.nowUtc());
TmchCrl.set(asciiCrl, url);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import google.registry.model.domain.token.AllocationToken.TokenType;
import google.registry.model.domain.token.BulkPricingPackage;
import google.registry.persistence.VKey;
import java.util.Date;
import google.registry.tools.params.DateTimeParameter;
import java.util.List;
import java.util.Optional;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -56,9 +56,11 @@ abstract class CreateOrUpdateBulkPricingPackageCommand extends MutatingCommand {
@Nullable
@Parameter(
names = "--next_billing_date",
converter = DateTimeParameter.class,
validateWith = DateTimeParameter.class,
description =
"The next date that the bulk pricing package should be billed for its annual fee")
Date nextBillingDate;
DateTime nextBillingDate;
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a bad change to me? Why are we going to make people specify an entire date time on the command line when the only thing that should be passed in is a date?


/** Returns the existing BulkPricingPackage or null if it does not exist. */
@Nullable
Expand Down Expand Up @@ -105,9 +107,7 @@ protected final void init() throws Exception {
Optional.ofNullable(maxCreates).ifPresent(builder::setMaxCreates);
Optional.ofNullable(price).ifPresent(builder::setBulkPrice);
Optional.ofNullable(nextBillingDate)
.ifPresent(
nextBillingDate ->
builder.setNextBillingDate(new DateTime(nextBillingDate)));
.ifPresent(nextBillingDate -> builder.setNextBillingDate(nextBillingDate));
if (clearLastNotificationSent()) {
builder.setLastNotificationSent(null);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import google.registry.module.backend.BackendRequestComponent;
import google.registry.module.bsa.BsaRequestComponent;
import google.registry.module.frontend.FrontendRequestComponent;
Expand All @@ -28,7 +29,6 @@
import java.util.ArrayList;
import java.util.List;
import org.junit.jupiter.api.Test;
import org.testcontainers.shaded.com.google.common.collect.ImmutableMap;

/** Unit tests for {@link RequestComponent}. */
public class RequestComponentTest {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import com.google.common.collect.Multimaps;
import com.google.common.net.HttpHeaders;
import com.google.common.net.MediaType;
import com.google.common.truth.Truth8;
import com.google.protobuf.Timestamp;
import com.google.protobuf.util.Timestamps;
import dagger.Module;
Expand Down Expand Up @@ -133,7 +132,7 @@ public List<Task> getTestTasksFor(String queue) {
public void assertTasksEnqueuedWithProperty(
String queueName, Function<Task, String> propertyGetter, String... expectedTaskProperties) {
// Ordering is irrelevant but duplicates should be considered independently.
Truth8.assertThat(getTestTasksFor(queueName).stream().map(propertyGetter))
assertThat(getTestTasksFor(queueName).stream().map(propertyGetter))
.containsExactly((Object[]) expectedTaskProperties);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ void testSuccess() throws Exception {
"--max_domains=100",
"--max_creates=500",
"--price=USD 1000.00",
"--next_billing_date=2012-03-17",
"--next_billing_date=2012-03-17T00:00:00Z",
"abc123");

Optional<BulkPricingPackage> bulkPricingPackageOptional =
Expand Down Expand Up @@ -161,7 +161,7 @@ void testSuccess_missingMaxDomainsAndCreatesInitializesToZero() throws Exception
.setAllowedEppActions(ImmutableSet.of(CommandName.CREATE))
.setDiscountFraction(1)
.build());
runCommandForced("--price=USD 1000.00", "--next_billing_date=2012-03-17", "abc123");
runCommandForced("--price=USD 1000.00", "--next_billing_date=2012-03-17T00:00:00Z", "abc123");
Optional<BulkPricingPackage> bulkPricingPackageOptional =
tm().transact(() -> BulkPricingPackage.loadByTokenString("abc123"));
assertThat(bulkPricingPackageOptional).isPresent();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ void testSuccess() throws Exception {
"--max_domains=200",
"--max_creates=1000",
"--price=USD 2000.00",
"--next_billing_date=2013-03-17",
"--next_billing_date=2013-03-17T00:00:00Z",
"--clear_last_notification_sent",
"abc123");

Expand Down Expand Up @@ -106,7 +106,7 @@ void testFailure_bulkPackageDoesNotExist() throws Exception {
"--max_domains=100",
"--max_creates=500",
"--price=USD 1000.00",
"--next_billing_date=2012-03-17",
"--next_billing_date=2012-03-17T00:00:00Z",
"nullPackage"));
Truth.assertThat(thrown.getMessage())
.isEqualTo("BulkPricingPackage with token nullPackage does not exist");
Expand All @@ -117,7 +117,7 @@ void testSuccess_missingMaxDomains() throws Exception {
runCommandForced(
"--max_creates=1000",
"--price=USD 2000.00",
"--next_billing_date=2013-03-17",
"--next_billing_date=2013-03-17T00:00:00Z",
"--clear_last_notification_sent",
"abc123");

Expand Down Expand Up @@ -159,7 +159,7 @@ void testSuccess_missingPrice() throws Exception {
runCommandForced(
"--max_domains=200",
"--max_creates=1000",
"--next_billing_date=2013-03-17",
"--next_billing_date=2013-03-17T00:00:00Z",
"--clear_last_notification_sent",
"abc123");

Expand Down
Loading