Use expiry(duration) with duration. #1223

mikereiche · 2021-09-14T05:46:20Z

Also adds test for exceptions thrown during events - with validator.

Closes #1204.

You have read the Spring Data contribution guidelines.
There is a ticket in the bug tracker for the project in our JIRA.
You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.
You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

daschl · 2021-09-14T06:34:46Z

pom.xml

@@ -118,6 +118,18 @@
            <optional>true</optional>
        </dependency>

+        <dependency>


are those really needed runtime dependencies? or do you use them for tests only? in which case they need to have test scope

daschl · 2021-09-14T06:35:12Z

...ain/java/org/springframework/data/couchbase/core/mapping/BasicCouchbasePersistentEntity.java

+	}
+
+	private static Duration getExpiryDuration(Expiry annotation, Environment environment) {
+		if (annotation == null)


missing { }

fixed in all three places.

daschl · 2021-09-14T06:37:13Z

@mikereiche does this supersede #1205 ?

daschl · 2021-09-14T06:38:10Z

@dnault can you take a look at the instant and duration related bits please?

mikereiche · 2021-09-14T06:20:41Z

src/main/java/org/springframework/data/couchbase/core/convert/MappingCouchbaseConverter.java

@@ -503,7 +503,7 @@ protected void writeInternal(final Object source, final CouchbaseDocument target
 		final TreeMap<Integer, String> suffixes = new TreeMap<>();
 		final TreeMap<Integer, String> idAttributes = new TreeMap<>();

-		target.setExpiration(entity.getExpiry());
+		target.setExpiration((int)(entity.getExpiryDuration().getSeconds()));



use explicit getExpiryDuration() as the other getExpiry() is sometimes duration and sometimes instant.

mikereiche · 2021-09-14T06:21:29Z

.../java/org/springframework/data/couchbase/core/CouchbaseTemplateKeyValueIntegrationTests.java

-		assertThrows(CouchbaseException.class, () -> couchbaseTemplate.findByQuery(UserSubmission.class).project(new String[] { "address.street" })
-				.withConsistency(QueryScanConsistency.REQUEST_PLUS).all());
+		assertThrows(CouchbaseException.class, () -> couchbaseTemplate.findByQuery(UserSubmission.class)
+				.project(new String[] { "address.street" }).withConsistency(QueryScanConsistency.REQUEST_PLUS).all());



formatting only

mikereiche · 2021-09-14T06:29:43Z

...st/java/org/springframework/data/couchbase/repository/query/StringN1qlQueryCreatorTests.java

 			ExecutableFindByQuery q = (ExecutableFindByQuery) couchbaseTemplate
-					.findByQuery(Airline.class).matching(query);
+					.findByQuery(Airline.class).withConsistency(QueryScanConsistency.REQUEST_PLUS).matching(query);



Do this properly with QueryScanConsistency instead of sleep()

Also adds test for exceptions thrown during events - with validator. Closes #1204.

mikereiche · 2021-09-14T06:53:12Z

...ain/java/org/springframework/data/couchbase/core/mapping/BasicCouchbasePersistentEntity.java

+		return Instant.ofEpochSecond(cal.getTimeInMillis() / 1000); // note: Unix UTC time representation in int is okay
+																																// until year 2038
+	}
+
 	private static int getExpiryValue(Expiry annotation, Environment environment) {


To avoid the ambiguity with getExpiry() that returns a long that is sometimes a seconds_duration and sometimes an instant_seconds_since_epoch.
getExpiryDuration() returns a Duration
getExpiryInstant() returns an Instant

mikereiche · 2021-09-14T06:54:11Z

.../java/org/springframework/data/couchbase/core/CouchbaseTemplateKeyValueIntegrationTests.java

@@ -84,6 +85,7 @@ public void beforeEach() {
 		couchbaseTemplate.removeByQuery(UserAnnotated.class).all();
 		couchbaseTemplate.removeByQuery(UserAnnotated2.class).all();
 		couchbaseTemplate.removeByQuery(UserAnnotated3.class).all();
+		couchbaseTemplate.removeByQuery(User.class).withConsistency(QueryScanConsistency.REQUEST_PLUS).all();
 	}


This fixed intermittent failures when my vagrant vm got slow.

mikereiche · 2021-09-14T06:54:53Z

.../java/org/springframework/data/couchbase/core/CouchbaseTemplateKeyValueIntegrationTests.java

@@ -282,15 +284,23 @@ void withExpiryAndExpiryAnnotation()
 		}
 		// check that they are gone after a few seconds.
 		sleepSecs(4);


This gives all the failures instead of the just the first one.

mikereiche · 2021-09-14T06:58:44Z

@mikereiche does this supersede #1205 ?

Yes. #1205 had a merge from main on top of it. I closed it now.

dnault · 2021-09-14T18:29:46Z

...ain/java/org/springframework/data/couchbase/core/mapping/BasicCouchbasePersistentEntity.java

+
+	private static Duration getExpiryDuration(Expiry annotation, Environment environment) {
+		if (annotation == null) {
+			return Duration.ofSeconds(0);


NIT: Could avoid creating a new Duration by using Duration.ZERO
Mike: Fixed

...ain/java/org/springframework/data/couchbase/core/mapping/BasicCouchbasePersistentEntity.java

dnault · 2021-09-14T18:33:49Z

src/main/java/org/springframework/data/couchbase/core/mapping/CouchbasePersistentEntity.java

+	/**
+	 * Returns the expiration time of the entity.
+	 * <p/>
+	 * The Couchbase format for expiration time is: - for TTL < 31 days (<= 30 * 24 * 60 * 60): expressed as a TTL in


NIT: Is this relevant when getting the expiry as a duration?

copied from getExpiry()

dnault · 2021-09-14T18:38:09Z

src/main/java/org/springframework/data/couchbase/core/mapping/CouchbasePersistentEntity.java

@@ -40,6 +43,26 @@
 	 */
 	int getExpiry();


Can getExpiry (and the static version) be deprecated?

Yes. It also needs to be replaced in ReactiveFindByIdOperationSupport where it is used for getAndTouch()

dnault · 2021-09-14T18:58:12Z

...ain/java/org/springframework/data/couchbase/core/mapping/BasicCouchbasePersistentEntity.java

+			return Instant.ofEpochSecond(0);
+		}
+		// we want it to be represented as a UNIX timestamp style, seconds since Epoch in UTC
+		Calendar cal = Calendar.getInstance(TimeZone.getTimeZone("UTC"));


What happens if the expiry unit is not DAYS or SECONDS? (I think this is also a problem in getExpiry where this code originated.)

Since we're not worried about time zones, and UTC does not have Daylight Savings, using a calendar seems like overkill. Why not just:

long currentEpochSecond = System.curentTimeMillis() / 1000; return Instant.ofEpochSecond(currentEpochSecond + secondsShift);

Removed this method.

If it's not days, then secondsShift is used which was produced from the expiryUnit() and expiryVlaue.
annotation.expiryUnit().toSeconds(expiryValue)
I don't understand the significance of adding in DAYS vs. seconds.

dnault · 2021-09-14T19:13:06Z

src/main/java/org/springframework/data/couchbase/core/mapping/CouchbasePersistentEntity.java

+	 *
+	 * @return the expiration time Instant
+	 */
+	Instant getExpiryInstant();


Is this method required? It seems like the value it returns is calculated from the duration specified by the user in the annotation.

If it's only used by integration tests, perhaps it could be moved into the test code?

mikereiche

I made a new PR for your comments.
It's getting close to the release so I was hasty getting the previous one merged.

mikereiche · 2021-09-14T20:54:22Z

...ain/java/org/springframework/data/couchbase/core/mapping/BasicCouchbasePersistentEntity.java

+			return Instant.ofEpochSecond(0);
+		}
+		// we want it to be represented as a UNIX timestamp style, seconds since Epoch in UTC
+		Calendar cal = Calendar.getInstance(TimeZone.getTimeZone("UTC"));


Removed this method.

mikereiche · 2021-09-14T21:00:24Z

src/main/java/org/springframework/data/couchbase/core/mapping/CouchbasePersistentEntity.java

@@ -40,6 +43,26 @@
 	 */
 	int getExpiry();


Yes. It also needs to be replaced in ReactiveFindByIdOperationSupport where it is used for getAndTouch()

mikereiche · 2021-09-14T21:00:43Z

src/main/java/org/springframework/data/couchbase/core/mapping/CouchbasePersistentEntity.java

+	/**
+	 * Returns the expiration time of the entity.
+	 * <p/>
+	 * The Couchbase format for expiration time is: - for TTL < 31 days (<= 30 * 24 * 60 * 60): expressed as a TTL in


copied from getExpiry()

mikereiche · 2021-09-14T21:14:08Z

src/main/java/org/springframework/data/couchbase/core/mapping/CouchbasePersistentEntity.java

+	 *
+	 * @return the expiration time Instant
+	 */
+	Instant getExpiryInstant();


mikereiche · 2021-09-14T21:30:53Z

...ain/java/org/springframework/data/couchbase/core/mapping/BasicCouchbasePersistentEntity.java

+			return Instant.ofEpochSecond(0);
+		}
+		// we want it to be represented as a UNIX timestamp style, seconds since Epoch in UTC
+		Calendar cal = Calendar.getInstance(TimeZone.getTimeZone("UTC"));


If it's not days, then secondsShift is used which was produced from the expiryUnit() and expiryVlaue.
annotation.expiryUnit().toSeconds(expiryValue)
I don't understand the significance of adding in DAYS vs. seconds.

mikereiche force-pushed the datacouch_1204_use_duration_for_expiry branch 10 times, most recently from 73f2b46 to 909d8f7 Compare September 14, 2021 06:30

daschl suggested changes Sep 14, 2021

View reviewed changes

daschl requested a review from dnault September 14, 2021 06:37

mikereiche force-pushed the datacouch_1204_use_duration_for_expiry branch 3 times, most recently from 6b44c7e to 80e69c0 Compare September 14, 2021 06:44

mikereiche commented Sep 14, 2021

View reviewed changes

Use expiry(duration) with duration.

a5f643b

Also adds test for exceptions thrown during events - with validator. Closes #1204.

mikereiche force-pushed the datacouch_1204_use_duration_for_expiry branch from 80e69c0 to a5f643b Compare September 14, 2021 06:55

mikereiche commented Sep 14, 2021

View reviewed changes

mikereiche merged commit a879d0b into main Sep 14, 2021

mikereiche deleted the datacouch_1204_use_duration_for_expiry branch September 14, 2021 17:11

dnault reviewed Sep 14, 2021

View reviewed changes

mikereiche mentioned this pull request Sep 14, 2021

address comments from expiry change #1229

Closed

mikereiche commented Sep 14, 2021

View reviewed changes

Use expiry(duration) with duration. #1223

Use expiry(duration) with duration. #1223

Uh oh!

Conversation

mikereiche commented Sep 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschl commented Sep 14, 2021

Uh oh!

daschl commented Sep 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikereiche Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikereiche commented Sep 14, 2021

Uh oh!

dnault Sep 14, 2021 • edited by mikereiche Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnault Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikereiche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikereiche Sep 14, 2021 •

edited

Loading

dnault Sep 14, 2021 •

edited by mikereiche

Loading

dnault Sep 14, 2021 •

edited

Loading