It appears innocent enough this change, but we can't be sure this won't break applications: isAttached() is a public method and it is valid to call inside load(). If someone depends on isAttached() during load() we will change this behavior.
Granted this new semantics are much more to my liking, and it fixes a bug. But does this justify the risk of breaking applications in other ways? I checked some of our projects for such mishaps but couldn't find any misuses of isAttached inside load(). Martijn On Mon, Nov 24, 2014 at 9:11 PM, Sven Meier (JIRA) <j...@apache.org> wrote: > > [ > https://issues.apache.org/jira/browse/WICKET-5772?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel > ] > > Sven Meier resolved WICKET-5772. > -------------------------------- > Resolution: Fixed > Fix Version/s: 6.19.0 > 7.0.0-M5 > > #attached is now set after calling #load(), as suggested. > > Thanks Martin! > >> LoadableDetachableModel caches null value if load() fails, bug in >> getObject() {attached = true;} >> ------------------------------------------------------------------------------------------------ >> >> Key: WICKET-5772 >> URL: https://issues.apache.org/jira/browse/WICKET-5772 >> Project: Wicket >> Issue Type: Bug >> Components: wicket >> Affects Versions: 1.4.23, 6.18.0, 7.0.0-M4 >> Reporter: Martin Makundi >> Assignee: Sven Meier >> Labels: easyfix, easytest, patch >> Fix For: 7.0.0-M5, 6.19.0 >> >> Original Estimate: 10m >> Remaining Estimate: 10m >> >> If you have a LoadableDetachableModel whose load() operation fails at an >> instance, the LoadableDetachableModel will cache null value because >> getObject() method sets attached = true; before load() invocation has >> returned. >> This results in methods trusting LoadableDetachableModel using the null >> value for their operations which is logically incorrect as null might not be >> a legal value at all. Such behavior would result in unexpected >> difficult-to-debug behavior in depending components. >> Easy fix: >> Move: >> attached = true; >> after line: >> transientModelObject = load(); >> Test case: >> /** >> * LoadableDetachableModel must not return null as an attached value if >> load() >> * fails. >> */ >> public void testWhenLoadFails() { >> final LoadableDetachableModel<Void> loadableDetachableModel = new >> LoadableDetachableModel<Void>() { >> /** >> * @see org.apache.wicket.model.LoadableDetachableModel#load() >> */ >> @Override >> protected Void load() { >> throw new UnsupportedOperationException("Let's assume this fails for >> some reason."); >> } >> }; >> try { >> loadableDetachableModel.getObject(); >> fail(UnsupportedOperationException.class + " expected."); >> } catch (final UnsupportedOperationException e) { >> LoggerFactory.getLogger(LoadableDetachableModel.class).debug("Expected >> behavior due to " + UnsupportedOperationException.class); >> } >> try { >> assertNotSame(LoadableDetachableModel.class + " should not return null >> if it's load() has failed\n", null, >> loadableDetachableModel.getObject()); >> } catch (final UnsupportedOperationException e) { >> LoggerFactory.getLogger(LoadableDetachableModel.class).debug("Expected >> behavior due to " + UnsupportedOperationException.class); >> } >> } > > > > -- > This message was sent by Atlassian JIRA > (v6.3.4#6332) -- Become a Wicket expert, learn from the best: http://wicketinaction.com
