CacheEntryEvent.getValue and getOldValue

[vbekiaris]

Observations from Joe Fialli on CacheEntryEvent following the resolution of #362 :

Here is the change that I noticed inconsistencies in.

public abstract class CacheEntryEvent<K, V> extends EventObject
implements Cache.Entry<K, V>
<deleted>
/**
* Returns the value stored in the cache when this entry was created.
* <p>
* The value will be available
* for {@link CacheEntryCreatedListener}, {@link CacheEntryExpiredListener}
* and {@link CacheEntryRemovedListener}
* if {@link CacheEntryListenerConfiguration#isOldValueRequired()} is true.
*
* @return the value corresponding to this entry
*/
public abstract V getValue();

Comments on current changes:

1. Since getValue method is overriding a method defined in Cache.Entry, an @Override annotation is missing.

2. getValue() is defined for {@link CacheEntryUpdateListener}, but that is not listed. Is the intent that getValue() always has a value for all CacheEntryUpdateListener, but its value being set for all other Listeners is based on CacheEntryListenerConfiguration#isOldValueRequired() being true?

   (2a). If answer to 2 is yes, I definitely agree that previous value for entry is not returned in CacheEntryExpiredListener and CacheEntryRemovedListener when isOldValueRequired is false. However, getValue() should always return entry's current value for CacheEntryCreatedListener regardless of CacheEntryListenerConfiguration.isOldValueRequired()

3. Current solution contradicts terminology for an existing Cache API remove method /**

   • Atomically removes the mapping for a key only if currently mapped to the
   • given value. ....
   • @param key key whose mapping is to be removed from the cache
   • @param oldValue value expected to be associated with the specified key */
     boolean javax.cache.Cache.remove(K key, V oldValue).

   In this existing method, the documentation refers to the value as oldValue (when it should for all intents be just "value"). This is a condition for the operation to occur. So the above parameter definitely should be "value" just as in a "Removed" or "Expired" Listener, the value should be considered the previous or "oldValue" prior to the entry no longer being associated with the cache.

4. The undocumented exception UnsupportedOperationExceotion that is thrown by the RI but not documented in spec or javadoc. This issue was raised in issue and not addressed before it was closed.

[vbekiaris]

I prepared a pull request for RI's implementation of CacheEventEntry.getOldValue to return null when old value is not available.

[jfialli]

For the sake of backwards compatibility, I can see why one would map oldValue to getValue() for EXPIRED and REMOVED CacheEntryEvent.

Minimal correction to javadoc that removes the erroneous reference to CacheEntryCreatedListener.

From:

• The value will be available
  • for {@link CacheEntryCreatedListener}, {@link CacheEntryExpiredListener}
  • and {@link CacheEntryRemovedListener}
  • if {@link CacheEntryListenerConfiguration#isOldValueRequired()} is true.

To:

• The value will be available
  • for {@link CacheEntryExpiredListener} and {@link CacheEntryRemovedListener}
  • when {@link CacheEntryListenerConfiguration#isOldValueRequired()} is true.

[cruftex]

@jfialli Thanks Joe for bringing this up. Here is indeed some unfinished business.

For the sake of backwards compatibility, I can see why one would map oldValue to getValue() for EXPIRED and REMOVED CacheEntryEvent.

Since it is in the concept that one listener implementation can serve for all events, e.g. for create, updated and removed, it would make sense to send the old value consistently via getOldValue(). Also it would simplify the understanding because now the term old value does not mean the same thing at every place. Or, in case there is still variations needed, then every listener type should get its own data type. We can only address this in 2.0. Sigh.

Clarifications: The clarifications above all make sense. There is one more in CacheEntryListenerConfiguration:

  /**
   * Determines if the old value should be provided to the
   * {@link CacheEntryListener}.
   *
   * @return <code>true</code> if the old value is required by the
   *         {@link CacheEntryListener}
   */
  boolean isOldValueRequired();

The spec wording follows rfc2119, so: should -> must

Unsopported Operation

The undocumented exception UnsupportedOperationExceotion that is thrown by the RI but not documented in spec or javadoc. This issue was raised in issue and not addressed before it was closed.

I am not totally convinced that "just" removing it from the RI is the best choice (as in https://github.com/jsr107/RI/pull/61). Since there is no TCK test, implementations will still do anything. The Spec doesn't say anything about when isOldValueRequired is false, so returning null is not in the Spec as well. There are two options I can think of:

1. We keep it like it is. If the old value is not required the behavior is not specified (and therefore implementation independent), in cases where the old value (not getOldValue() ....) is requested by the application.

2. An implementation may always provide the old value. If the old value is not required and not provided by the application an UnsupportedOperationException must be thrown and getOldValueVailable is false. For the created event this is always the case. The pattern is either do the right thing or complain. In case implementations need to change, it's quite easy to comply. The TCK could check both cases, if we want.

Thoughts?

[vbekiaris]

On the topic of UnsupportedOperationException, here is the javadoc of CacheEntryEvent.getOldValue:

  /**
   * Returns the previous value, that existed prior to the
   * modification of the Entry value. The old value will be available
   * for {@link CacheEntryUpdatedListener}
   * if {@link CacheEntryListenerConfiguration#isOldValueRequired()} is true.
   *
   * @return the previous value or <code>null</code> if there was no previous value
   */
  public abstract V getOldValue();

if there was no previous value in the @return javadoc can be interpreted as: (a) previous value is not applicable, for example there is no previous value in a CREATED event (b) previous value is not available (either due to (a) or because isOldValueRequired was false)

If the interpretation is (a), then any implementation-specific behavior is acceptable when isOldValueRequired == false (as is now). If the interpretation is (b) then RI must return null and so must every implementation when isOldValueRequired == false --> a TCK test should be created to enforce that.

Additionally, the method's javadoc above indicates The old value will be available for {@link CacheEntryUpdatedListener} if {@link CacheEntryListenerConfiguration#isOldValueRequired()} is true., so my understanding is that the intent here was rather (b).

[cruftex]

I see it practical. The spec has room for interpretation, for example getValue() ist not allowed to return null, the RI is doing it different, there is no TCK test and every implementation out in the wild is doing something, which we don't know. If we make the Spec and the TCK stricter, people need to change their code, implementations and applications. We should have a very good reasoning about it, or leave it as it is.

Additionally, the method's javadoc above indicates The old value will be available for {@link CacheEntryUpdatedListener} if {@link CacheEntryListenerConfiguration#isOldValueRequired()} is true., so my understanding is that the intent here was rather (b).

That's a wrong conclusion of the form (I just looked it up....) described here: https://en.wikipedia.org/wiki/Denying_the_antecedent

Nothing in the Spec says that an implementation is not allowed to send the old value in an event if isOldValueRequired is set to false. The intention of this option is not to switch the sending of the old value off, but to ensure it is send, in case the applications relies on it. In case it is not required a cache implementation may choose not to send it, for example to save network bandwidth.

[cruftex]

To be fair I add another option to the list above:

1. We keep it like it is. If the old value is not required the behavior is not specified (and therefore implementation independent), in cases where the old value (not getOldValue() ....) is requested by the application.

2. An implementation may always provide the old value. If the old value is not required and not provided by the implementation an UnsupportedOperationException must be thrown and getOldValueVailable is false. For the created event this is always the case. The pattern is either do the right thing or complain. In case implementations need to change, it's quite easy to comply. The TCK could check both cases, if we want.

3. An implementation may always provide the old value. If the old value is not required and not provided by the implementation null is returned.

In terms of good API design I am still in favor of 2, because:

• If an application wants to check whether the old value is available it can use isOldValueAvailable() and should not use things like getOldValue() != null. In a code inspection the semantics would be interpreted identical, which is not the case.
• The UnsupportedException can be more descriptive and contain more information exactly at the point of the problem rather then a NPE further down.

[vbekiaris]

Nothing in the Spec says that an implementation is not allowed to send the old value in an event if isOldValueRequired is set to false.

I agree, I offered an interpretation attempting to define what is not already defined, not proof. I agree that solution 2 is cleaner API but have some reservations about introducing the exception throwing behavior in a maintenance release.

[cruftex]

I agree that solution 2 is cleaner API but have some reservations about introducing the exception throwing behavior in a maintenance release.

Sometimes I like to be more perfect but it isn't possible. In this case implementations diverge because the Spec isn't clear and there is no TCK test. Checking the implementations everyone that implemented some logic around "old value" needs to change as soon as the spec and the tests get stricter, no matter whether we decide for option 2 or 3.

I browsed trough all implementations I monitor via the test zoo project. There are three variants:

• Not affected, isOldValueAvailable() is always true, but if it means getOldValue is populated as written this is wrong also
• Null, but isOldValueAvailable only applies for the update case, because isOldValueAvailable() { return this.oldValue != null; }
• Exception in getOldValue but null in getValue, copy from the RI

I think we need/should to do a step back here, because I am not sure whether we really have consensus what isOldValueAvailable() means and/or whether we really identified all areas that cause pain and confusion.

• Does isOldValueAvailable() apply only to the method getOldValue() or to the concept that the previous value in the cache is send, which could be either in getOldValue() for an update or in getValue() in removed or expired event. If the latter, that means that "old" means two different things in the same interface....

• Does isOldValueRequired() only apply for update or removed and expired as well?

• What should the outcome of getOldValue() be if it makes logically no sense to have one, in the created, removed and expired event? Remark: It might lead to an exception or null or be identical to getValue() for removed and expired.

• What should the outcome of getValue() or getOldValue() be if the cacheimplementation (legally) decides that it is not available?

To make things easier we should try first whether we can agree on semantics for the case isOldValueRequired() == true.

[jfialli]

I am getting lost in the prose descriptions. Here is an attempt at a systematic treatment.

For the tables below, value represents a value that is in the cache while the Listener is running and oldValue represents the previous value that is no longer in the cache when Listener is executing.

Here is a table when CacheEntryListenerConfiguration#isOldValueRequired() is true that summarizes the ideal solution based on the names involved.

CacheEntryEvent Method	CreatedListener	UpdatedListener	Removed/Expired Listener
getValue()	value	value	null
getOldValue()	null	oldValue	oldValue

Note the use of past tense for CacheEntryExpiredListener and CacheEntryRemovedListener. OldValue was definitely meant to stand for a value that was no longer in the active cache.

Here is table when CacheEntryListenerConfiguration#isOldValueRequired is false:

CacheEntryEvent Method	CreatedListener	UpdatedListener	Removed/Expired Listener
getValue()	value	value	null
getOldValue()	null	null	null

Only the value that is still in the cache is accessible via getValue(). The above definition saves the most network bandwidth if one is looking to avoid sending data that is not being used.The Listeners would only have access to data that is in cache.

To be backwards compatible with 1.0, the following could be a compromise when isOldValueRequired() == true.

CacheEntryEvent Method	CreatedListener	UpdatedListener	Removed/Expired Listener
getValue()	value	value	oldValue
getOldValue()	null	oldValue	oldValue

For the sake of backwards compatibility, getValue() in Removed/ExpiredListener can be documented to return the value of getOldValue(). With this proposal, one can document that isOldValueRequired only refers to previous/old values that are no longer in the cache to being optimized out of CacheEvents. It is only for backwards compatibiliity purposes with 1.0 that getValue() is returning oldValue in CacheEventEntry sent to Removed/ExpiryListener.

[cruftex]

I am getting lost in the prose descriptions. Here is an attempt at a systematic treatment.

Thanks Joe. Your tables are a relief.

CacheEntryEvent Method	CreatedListener	UpdatedListener	Removed/Expired Listener
getValue()	value	value	oldValue
getOldValue()	null	oldValue	oldValue

For the sake of backwards compatibility, getValue() in Removed/ExpiredListener can be documented to return the value of getOldValue().

Good compromise!

Here is table when CacheEntryListenerConfiguration#isOldValueRequired is false:

CacheEntryEvent Method	CreatedListener	UpdatedListener	Removed/Expired Listener
getValue()	value	value	null
getOldValue()	null	null	null

Only the value that is still in the cache is accessible via getValue().

Not so fast. That is not in the wording nor the meaning of the Spec yet. If it is not required it means:

Updated table when CacheEntryListenerConfiguration#isOldValueRequired is false:

CacheEntryEvent Method	CreatedListener	UpdatedListener	Removed/Expired Listener
getValue()	value	value	oldValue or null
getOldValue()	null	oldValue or null	oldValue or null

W.R.T.:

The above definition saves the most network bandwidth if one is looking to avoid sending data that is not being used.The Listeners would only have access to data that is in cache.

I have two different notions of "configure". The first is enable and optimize a specific cache implementation in a specific environment, the second is that an application is requesting specific semantics that it needs to function properly. The second one we need on the API level. The first one is intentionally not addressed by JSR107.

[jfialli]

I agree with the updated table when isOldValueRequired is false.

[vbekiaris]

Thanks Joe & Jens, I also think the tables you agreed on make sense.

[vbekiaris]

Proposal for updated javadoc on CacheEntryEvent methods based on tables above:

/**
   * Returns the value stored in the cache when this entry was created.
   * <p>
   * The value will be available
   * for {@link CacheEntryCreatedListener} and {@link CacheEntryUpdatedListener}.
   * For backwards compatibility, this method will return the value of
   * {@link #getOldValue()} for {@link CacheEntryExpiredListener}
   * and {@link CacheEntryRemovedListener}.
   *
   * @return the value corresponding to this entry
   * @see #getOldValue()
   */
  public abstract V getValue();

  /**
   * Returns the previous value, that existed prior to the
   * modification of the Entry value. The old value will be available
   * for {@link CacheEntryUpdatedListener}, {@link CacheEntryExpiredListener}
   * and {@link CacheEntryRemovedListener}
   * if {@link CacheEntryListenerConfiguration#isOldValueRequired()} is true.
   * The old value may be available for {@link CacheEntryUpdatedListener},
   * {@link CacheEntryExpiredListener} and {@link CacheEntryRemovedListener}
   * if {@link CacheEntryListenerConfiguration#isOldValueRequired()} is false.
   *
   * @return the previous value or <code>null</code> if there was no previous value or
   * the previous value is not available
   */
  public abstract V getOldValue();

  /**
   * Whether the old value is available. The old value will be available
   * for {@link CacheEntryUpdatedListener}, {@link CacheEntryExpiredListener}
   * and {@link CacheEntryRemovedListener}
   * if {@link CacheEntryListenerConfiguration#isOldValueRequired()} is true. 
   *
   * @return true if the old value is populated
   */
  public abstract boolean isOldValueAvailable();

@jfialli @cruftex comments? Once we finalize this I'll prepare pull requests for spec, RI and TCK as all have to be updated.

[vbekiaris]

Pull requests:

• spec: #392
• RI: https://github.com/jsr107/RI/pull/62
• TCK: https://github.com/jsr107/jsr107tck/pull/133. Apart from the adaptation for the updated spec, this PR includes a fix for the client server test infrastructure; until now, AssertionErrors thrown from a CacheEntryListener (eg CacheTestSupport$MyCacheEntryListener) were swallowed.

[jfialli]

Add @Override annotation to getValue(). (Its is defined initially in Cache.Entry).

---

getOldValue documentation update.

Change: Returns the previous value, that existed prior to the modification of the Entry value To: Returns the previous value that existed for entry in the cache before modification or removal.

[vbekiaris]

@jfialli thanks, I updated the pull request with these changes in a separate commit (will squash commits before merge): https://github.com/jsr107/jsr107spec/pull/392/commits/251cbaffd1c21306568dc5109255ae511a6e7798

[cruftex]

consequently, getValue() documentation update.

Change: Returns the value stored in the cache when this entry was created. To: Returns the value stored in the cache when this entry was created or updated.

"For backwards compatibility..."

True, but irritating. Better remove the rationale, since the "why" is usually not addressed in the JavaDoc. Better we add what API clients should or should not do. Actually, its quite painful. My try of it:

Returns the the same value of {@link #getOldValue()} for {@link CacheEntryExpiredListener} and {@link CacheEntryRemovedListener}. Cache clients that need to maintain compatibility with JSR107 version 1.0 cache implementations, need to use this method for retrieving the expired or removed value. After the used cache implementations are compatible to JSR107 version 1.1, clients should prefer the method {@link #getOldValue()}

isOldValueAvailable:

The old value will be available for {@link CacheEntryUpdatedListener}, {@link CacheEntryExpiredListener} and {@link CacheEntryRemovedListener} if {@link CacheEntryListenerConfiguration#isOldValueRequired()} is true.

Add:

The old value may be available for {@link CacheEntryUpdatedListener}, {@link CacheEntryExpiredListener} and {@link CacheEntryRemovedListener} if {@link CacheEntryListenerConfiguration#isOldValueRequired()} is false.

Or remove it and refer to getOldValue. But if you address an aspect, it must be complete.

[vbekiaris]

Returns the value stored in the cache when this entry was created or updated.

👍

Better remove the rationale, since the "why" is usually not addressed in the JavaDoc.

Agreed. I did some minor editing to the text you proposed, here is the result:

  /**
   * Returns the value stored in the cache when this entry was created or updated.
   * <p>
   * The value will be available
   * for {@link CacheEntryCreatedListener} and {@link CacheEntryUpdatedListener}.
   * Returns the same value as {@link #getOldValue()} for {@link CacheEntryExpiredListener}
   * and {@link CacheEntryRemovedListener}. Cache clients that need to maintain compatibility with
   * JSR107 version 1.0 cache implementations, need to use this method for retrieving the expired
   * or removed value. When using cache implementations compatible with JSR107 version 1.1,
   * clients should prefer the method {@link #getOldValue()}.
   *
   * @return the value corresponding to this entry
   * @see #getOldValue()
   */
  @Override
  public abstract V getValue();

And here is isOldValueAvailable() in its entirety - apart from adding the missing isOldValueRequired==false aspect (well spotted, thanks), I also changed the @return statement so that it uses the same terminology (available) instead of introducing another term (populated):

  /**
   * Whether the old value is available. The old value will be available
   * for {@link CacheEntryUpdatedListener}, {@link CacheEntryExpiredListener}
   * and {@link CacheEntryRemovedListener}
   * if {@link CacheEntryListenerConfiguration#isOldValueRequired()} is true.
   * The old value may be available for {@link CacheEntryUpdatedListener},
   * {@link CacheEntryExpiredListener} and {@link CacheEntryRemovedListener}
   * if {@link CacheEntryListenerConfiguration#isOldValueRequired()} is false.
   *
   * @return true if the old value is available
   */
  public abstract boolean isOldValueAvailable();

What do you think?

[cruftex]

I just merged the PR. It includes all what we discussed above. Now the Spec document should be updated. I like to keep this issue open until we finalized the changes to the RI and TCK and Joe and Greg gave their +1.

[gregrluck]

This should be closed. Merges were done back in October.