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 jsr107/RI#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.