This is an automated email from the ASF dual-hosted git repository. reiern70 pushed a commit to branch improve-ldms in repository https://gitbox.apache.org/repos/asf/wicket.git
commit 9ce30a9d07478b575d9dc549235318e628dabd45 Author: reiern70 <[email protected]> AuthorDate: Wed Sep 23 09:57:28 2026 -0500 Seal LoadableDetachableModel#detach() and add object-aware detach hooks detach() drives the model's attach/detach state machine: it invokes onDetach() when there is something to detach, then discards the transient object and resets the state. It was overridable, so a subclass could run cleanup on either side of super.detach(), or skip the super call and leave the model attached for good. Nothing enforced the invariant the method exists to maintain. detach() is now final, and cleanup goes into one of two hooks: onDetach() - as before, only when the model was attached onDetachAlways() - on every detach() call, attached or not onDetachAlways() is what an override of detach() in practice was, and is where cleanup not tied to the loaded object belongs: detaching models this one was handed, for instance, which may have been attached without this model ever loading. A subclass that needed the loaded object while detaching had to keep a reference of its own, because onDetach() took no arguments and the field holding the object is private. The new onDetach(T object) is handed that object before it is discarded. The default onDetach() delegates to it, so an override of onDetach() that does not call super suppresses it. Making a public method final is source-incompatible, which is why this lands on master only. It stays binary compatible - detach() still resolves, on LoadableDetachableModel - so compiled subclasses keep running, but a subclass that overrides detach() no longer compiles. Moving the override's body to onDetachAlways() and dropping the super.detach() call reproduces the old behaviour exactly; moving it to onDetach() narrows it to the attached case. That rewrite needs a judgement about which hook applies, so it is documented as a manual step in wicket.yml rather than automated. Three subclasses in the tree overrode detach(), each to detach something unconditionally, and all three move to onDetachAlways(): StringResourceModel and its AssignmentWrapper, and the devutils SessionIdentifiersModel. StringResourceModel is the reason onDetachAlways() exists at all - per WICKET-5176 it has to detach its substitution models even when it was never attached itself, which an attached-only hook cannot do. GitHub issue #1614: https://github.com/apache/wicket/issues/1614 --- .../wicket/model/LoadableDetachableModelTest.java | 112 +++++++++++++++++++++ .../wicket/model/LoadableDetachableModel.java | 41 +++++++- .../apache/wicket/model/StringResourceModel.java | 23 +++-- .../pagestore/browser/SessionIdentifiersModel.java | 4 +- .../src/main/resources/META-INF/rewrite/wicket.yml | 8 ++ 5 files changed, 175 insertions(+), 13 deletions(-) diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/model/LoadableDetachableModelTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/model/LoadableDetachableModelTest.java index 6035a083de..8c23939123 100644 --- a/wicket-core-tests/src/test/java/org/apache/wicket/model/LoadableDetachableModelTest.java +++ b/wicket-core-tests/src/test/java/org/apache/wicket/model/LoadableDetachableModelTest.java @@ -18,6 +18,7 @@ package org.apache.wicket.model; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import java.io.ByteArrayInputStream; @@ -134,6 +135,117 @@ class LoadableDetachableModelTest extends WicketTestCase assertEquals(true, ldm.detachCalled); } + @Test + void onDetachReceivesTheAttachedObject() + { + class DetachingLoad extends LoadableDetachableModel<Integer> + { + private static final long serialVersionUID = 1L; + + private boolean detachCalled = false; + + private Integer detached; + + @Override + protected Integer load() + { + return 42; + } + + @Override + protected void onDetach(Integer object) + { + detachCalled = true; + detached = object; + } + } + + DetachingLoad ldm = new DetachingLoad(); + assertThat(ldm.getObject()).isEqualTo(42); + + ldm.detach(); + + assertEquals(true, ldm.detachCalled); + assertThat(ldm.detached).isEqualTo(42); + assertEquals(false, ldm.isAttached()); + } + + @Test + void onDetachReceivesNullAfterFailedLoad() + { + class ExceptionalLoad extends LoadableDetachableModel<Integer> + { + private static final long serialVersionUID = 1L; + + private boolean detachCalled = false; + + private Integer detached = 42; + + @Override + protected Integer load() + { + throw new RuntimeException(); + } + + @Override + protected void onDetach(Integer object) + { + detachCalled = true; + detached = object; + } + } + + ExceptionalLoad ldm = new ExceptionalLoad(); + assertThrows(RuntimeException.class, ldm::getObject); + + ldm.detach(); + + assertEquals(true, ldm.detachCalled); + assertThat(ldm.detached).isNull(); + } + + @Test + void onDetachAlwaysCalledEvenWhenNeverAttached() + { + class CountingLoad extends LoadableDetachableModel<Integer> + { + private static final long serialVersionUID = 1L; + + private int detachCount = 0; + + private int alwaysCount = 0; + + @Override + protected Integer load() + { + return 42; + } + + @Override + protected void onDetach(Integer object) + { + detachCount++; + } + + @Override + protected void onDetachAlways() + { + alwaysCount++; + } + } + + CountingLoad ldm = new CountingLoad(); + + ldm.detach(); + assertEquals(0, ldm.detachCount); + assertEquals(1, ldm.alwaysCount); + + ldm.getObject(); + ldm.detach(); + assertEquals(1, ldm.detachCount); + assertEquals(2, ldm.alwaysCount); + } + private static class SerializedLoad extends LoadableDetachableModel<Integer> { private static final long serialVersionUID = 1L; diff --git a/wicket-core/src/main/java/org/apache/wicket/model/LoadableDetachableModel.java b/wicket-core/src/main/java/org/apache/wicket/model/LoadableDetachableModel.java index 872a202ba2..d36464046e 100644 --- a/wicket-core/src/main/java/org/apache/wicket/model/LoadableDetachableModel.java +++ b/wicket-core/src/main/java/org/apache/wicket/model/LoadableDetachableModel.java @@ -98,7 +98,7 @@ public abstract class LoadableDetachableModel<T> implements IModel<T> } @Override - public void detach() + public final void detach() { // even if LDM is in partial attached state (ATTACHING) it should be detached if (state != null && state != InternalState.DETACHED) @@ -115,6 +115,8 @@ public abstract class LoadableDetachableModel<T> implements IModel<T> log.debug("removed transient object for '{}'", this); } } + + onDetachAlways(); } @Override @@ -178,8 +180,45 @@ public abstract class LoadableDetachableModel<T> implements IModel<T> /** * Detaches from the current request. Implement this method with custom behavior, such as * setting the model object to null. + * <p> + * This implementation delegates to {@link #onDetach(Object)}, passing the object this model is + * currently holding. An override that does not call {@code super.onDetach()} suppresses that + * callback. */ protected void onDetach() + { + onDetach(transientModelObject); + } + + /** + * Detaches from the current request, handing over the object this model is holding before it is + * discarded. This is the place to release resources tied to that object. + * <p> + * The object is the one returned by the last {@link #load()} or passed to + * {@link #setObject(Object)}. It is {@code null} when that value was {@code null}, and also when + * the model is detached while still attaching - {@link #load()} is then either running or has + * thrown. + * + * @param object + * the object that was attached, may be {@code null} + * @since 11.0.0 + */ + protected void onDetach(T object) + { + } + + /** + * Detaches from the current request, whether this model was attached or not. Unlike + * {@link #onDetach()} this is invoked on every call to {@link #detach()}, so it is the place for + * cleanup that is not tied to the loaded object - detaching models this one was handed, for + * instance, which may have been attached without this model ever loading. + * <p> + * When the model was attached, this runs after {@link #onDetach()} and after the loaded object + * has been discarded. + * + * @since 11.0.0 + */ + protected void onDetachAlways() { } diff --git a/wicket-core/src/main/java/org/apache/wicket/model/StringResourceModel.java b/wicket-core/src/main/java/org/apache/wicket/model/StringResourceModel.java index 2364fa2aa3..daf7402683 100644 --- a/wicket-core/src/main/java/org/apache/wicket/model/StringResourceModel.java +++ b/wicket-core/src/main/java/org/apache/wicket/model/StringResourceModel.java @@ -241,23 +241,26 @@ public class StringResourceModel extends LoadableDetachableModel<String> this.component = component; } - @Override - public void detach() - { - super.detach(); - - StringResourceModel.this.detach(); - } - @Override protected void onDetach() { + super.onDetach(); + if (StringResourceModel.this.component == null) { + // without an explicit component the wrapped model never attaches itself StringResourceModel.this.onDetach(); } } + @Override + protected void onDetachAlways() + { + super.onDetachAlways(); + + StringResourceModel.this.detach(); + } + @Override protected String load() { @@ -619,9 +622,9 @@ public class StringResourceModel extends LoadableDetachableModel<String> } @Override - public final void detach() + protected void onDetachAlways() { - super.detach(); + super.onDetachAlways(); // detach any model if (model != null) diff --git a/wicket-devutils/src/main/java/org/apache/wicket/devutils/pagestore/browser/SessionIdentifiersModel.java b/wicket-devutils/src/main/java/org/apache/wicket/devutils/pagestore/browser/SessionIdentifiersModel.java index 7cd8b4c189..b7183582ca 100644 --- a/wicket-devutils/src/main/java/org/apache/wicket/devutils/pagestore/browser/SessionIdentifiersModel.java +++ b/wicket-devutils/src/main/java/org/apache/wicket/devutils/pagestore/browser/SessionIdentifiersModel.java @@ -63,9 +63,9 @@ public class SessionIdentifiersModel extends LoadableDetachableModel<List<String } @Override - public void detach() + protected void onDetachAlways() { - super.detach(); + super.onDetachAlways(); store.detach(); } diff --git a/wicket-migration/src/main/resources/META-INF/rewrite/wicket.yml b/wicket-migration/src/main/resources/META-INF/rewrite/wicket.yml index 90b1b3a735..7bd0e14305 100644 --- a/wicket-migration/src/main/resources/META-INF/rewrite/wicket.yml +++ b/wicket-migration/src/main/resources/META-INF/rewrite/wicket.yml @@ -81,6 +81,14 @@ recipeList: # The ChangeType entries below handle the common case; getRememberMe(), setRememberMe(), # onSignInRemembered() and the two-argument SignInPanel constructor have no equivalent and need # hand-editing. Both are deprecated in 8.19.0, 9.24.0 and 10.11.0. +# +# org.apache.wicket.model.LoadableDetachableModel#detach() is final, so a subclass can no longer +# step into the attach/detach state machine. An override of it moves its body to one of two hooks +# and drops the super.detach() call: onDetachAlways(), which runs on every detach() just as the +# override did, or onDetach(), which runs only when the model was attached. The new onDetach(T) is +# handed the object the model was holding before that object is discarded; the default onDetach() +# delegates to it, so an override of onDetach() that does not call super suppresses it. These +# require manual migration. type: specs.openrewrite.org/v1beta/recipe name: org.apache.wicket.MigrateToWicket11 displayName: Migrate to Wicket 11.x
