wicket-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pedro Santos <pedros...@gmail.com>
Subject Re: overriding isVisible bad?
Date Tue, 30 Nov 2010 19:44:31 GMT
I have a different point of view, the HTTP imposes us some limitations, we
will hardly have an good synchronization between the component state on
browser and server using only HTTP conversation. So it is mandatory the
service layer to respect the described security restriction.

On Tue, Nov 30, 2010 at 5:32 PM, Igor Vaynberg <igor.vaynberg@gmail.com>wrote:

> an easy example is security.
>
> a user views a page that allows them to delete another user
> meanwhile their permissions are tweaked and they can no longer delete
> other users
> half an hour later the user clicks the delete button - this should
> fail, but wont if we are using last-rendered state.
>
> -igor
>
> On Tue, Nov 30, 2010 at 11:18 AM, Pedro Santos <pedrosans@gmail.com>
> wrote:
> > I need to look better on which core components are relying on an updated
> > visibility/enabled state at the process event time, and why the last
> > rendered state wouldn't be enough to them to work nicely.
> >
> > On Tue, Nov 30, 2010 at 3:19 PM, Igor Vaynberg <igor.vaynberg@gmail.com
> >wrote:
> >
> >> currently we only invoke configure before the render. this would mean
> >> we would have to invoke it before processing a listener, clearing the
> >> cache, and then invoking it again before render. i wonder if that is
> >> enough places to invoke it....
> >>
> >> -igor
> >>
> >> On Tue, Nov 30, 2010 at 9:15 AM, Pedro Santos <pedrosans@gmail.com>
> wrote:
> >> > If user click an link, it will change the value of some property at
> the
> >> > process_event request cycle step. Then the processor will go to the
> >> respond
> >> > step, will invoke every component before render method which will end
> up
> >> > invoking the Component#configure and updating the visibility/enabled
> >> state
> >> > (even if it changes, we are able to work with the updated state). So
> when
> >> > the this component has the opportunity to render it self, it will be
> >> aware
> >> > its update state.
> >> >
> >> > On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg <
> igor.vaynberg@gmail.com
> >> >wrote:
> >> >
> >> >> there are other places that should be checked though. for example
> >> >> before we invoke a listener on the component we should check again
to
> >> >> make sure that visibility hasnt changed. eg if visibility depends on
> >> >> some property of the user clicking the link that changed between
> >> >> render and clicking the link.
> >> >>
> >> >> -igor
> >> >>
> >> >> On Tue, Nov 30, 2010 at 8:30 AM, Pedro Santos <pedrosans@gmail.com>
> >> wrote:
> >> >> > An implementation idea:
> >> >> >
> >> >> > Component {
> >> >> >    public final void configure()
> >> >> >    {
> >> >> >        if (!getFlag(FLAG_CONFIGURED))
> >> >> >        {
> >> >> >            setVisible_NoClientCode(isVisible()); //we only check
> the
> >> user
> >> >> > isVisible in here
> >> >> >            onConfigure();
> >> >> >            setFlag(FLAG_CONFIGURED, true);
> >> >> >        }
> >> >> >    }
> >> >> > }
> >> >> >
> >> >> > On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg <
> >> igor.vaynberg@gmail.com
> >> >> >wrote:
> >> >> >
> >> >> >> so how is it different if they can still override something
that
> >> needs
> >> >> >> to be checked all the time?
> >> >> >>
> >> >> >> -igor
> >> >> >>
> >> >> >> On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos <
> pedrosans@gmail.com>
> >> >> wrote:
> >> >> >> > I understand the concern about possible isVisible
> implementations
> >> like
> >> >> >> >
> >> >> >> > isVisible(return currentlyTime < 10:00:00;) //imagine
this
> >> component
> >> >> >> being
> >> >> >> > rendered at 09:59:59
> >> >> >> > isVisible(return dao.list().size() > 0);// performance
issues
> >> >> >> >
> >> >> >> > But maybe we can have the best from both approaches.
This is an
> >> >> >> copy/paste
> >> >> >> > from java.awt.Component:
> >> >> >> >
> >> >> >> >    public boolean isVisible() {
> >> >> >> >        return isVisible_NoClientCode();
> >> >> >> >    }
> >> >> >> >    final boolean isVisible_NoClientCode() {
> >> >> >> >        return visible;
> >> >> >> >    }
> >> >> >> >
> >> >> >> > There are some points in the awt framework were the isVisible
> >> method
> >> >> is
> >> >> >> not
> >> >> >> > used in benefit of isVisible_NoClientCode
> >> >> >> > I'm in favor of create an final isVisible/Enabled version
and
> >> change
> >> >> the
> >> >> >> > Wicket core to use it. Also maintain the hotspot to users
> provide
> >> >> their
> >> >> >> > isVisible/Enable implementations that will serve to feed
the
> core
> >> >> >> component
> >> >> >> > state.
> >> >> >> >
> >> >> >> > On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg <
> >> >> igor.vaynberg@gmail.com
> >> >> >> >wrote:
> >> >> >> >
> >> >> >> >> ive run into plenty of weird problems with overrides,
but maybe
> >> >> >> >> because this was in a high concurrency app where
data changed
> >> >> >> >> frequently. the problems arise from the fact that
the value
> >> returned
> >> >> >> >> from isvisible() can change while we are doing traversals,
etc.
> >> >> >> >>
> >> >> >> >> eg we run a traversal for all visible components
and put them
> in a
> >> >> >> >> list. later we iterate over the list and try to render
these
> >> >> >> >> components. the render function also checks their
visibility
> and
> >> if
> >> >> >> >> they are no longer visible it throws an exception.
> >> >> >> >>
> >> >> >> >> if isvisible() override depends on some external
factor like
> the
> >> >> >> >> database there is a small window where the value
can change and
> >> now
> >> >> >> >> you can have a weird exception: such as "tried to
invoke a
> >> listener
> >> >> on
> >> >> >> >> a component that is not visible or not enabled".
these are very
> >> >> >> >> intermittent and damn near impossible to reproduce.
> >> >> >> >>
> >> >> >> >> another problem is performance. isvisible() is called
multiple
> >> times
> >> >> >> >> during the request and if it depends on the database
it can be
> a
> >> >> >> >> performance problem. in fact a couple of users have
complained
> >> about
> >> >> >> >> this on the list in the past. at least now we have
an easy
> >> solution
> >> >> >> >> for them - use onconfigure().
> >> >> >> >>
> >> >> >> >> so as of right now the developers have two choices:
override
> >> >> >> >> isvisible() and potentially suffer the consequences.
or,
> override
> >> >> >> >> onconfigure() and set visibility there in a more
deterministic
> >> >> >> >> fashion.
> >> >> >> >>
> >> >> >> >> -igor
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> On Mon, Nov 29, 2010 at 10:21 AM, Eelco Hillenius
> >> >> >> >> <eelco.hillenius@gmail.com> wrote:
> >> >> >> >> > To expand, unless I'm missing something (new?),
things are
> >> really
> >> >> only
> >> >> >> >> > problematic when both the mutable value and
the override are
> >> mixed.
> >> >> In
> >> >> >> >> > a way, I think that using the override is 'more
pure', as
> it's a
> >> >> >> >> > simple function that is executed when needed,
whereas mutable
> >> state
> >> >> >> >> > can be harder to deal with when trying to figure
out how it
> got
> >> to
> >> >> be
> >> >> >> >> > in that state. So, sorry Igor, but we disagree
on this one.
> >> >> >> >> >
> >> >> >> >> > Eelco
> >> >> >> >> >
> >> >> >> >> >
> >> >> >> >> > On Mon, Nov 29, 2010 at 10:13 AM, Eelco Hillenius
> >> >> >> >> > <eelco.hillenius@gmail.com> wrote:
> >> >> >> >> >> Niether is evil. It has potential pitfalls,
which you should
> >> just
> >> >> be
> >> >> >> >> >> aware of. We use such overrides all over
the place and never
> >> have
> >> >> >> >> >> problems with them either. :-) Avoiding
it is safer, but
> also
> >> more
> >> >> >> >> >> verbose (in 1.3.x at least).
> >> >> >> >> >>
> >> >> >> >> >> Eelco
> >> >> >> >> >>
> >> >> >> >> >> On Mon, Nov 29, 2010 at 9:49 AM, Igor Vaynberg
<
> >> >> >> igor.vaynberg@gmail.com>
> >> >> >> >> wrote:
> >> >> >> >> >>> On Mon, Nov 29, 2010 at 9:35 AM, Sven
Meier <
> sven@meiers.net>
> >> >> >> wrote:
> >> >> >> >> >>>> Hi Douglas,
> >> >> >> >> >>>>
> >> >> >> >> >>>> WICKET-3171 describes a problematic
case, where visibility
> of
> >> a
> >> >> >> >> >>>> component changes while its form
is being processed.
> >> >> >> >> >>>> In our projects we're overriding
isVisible() where
> >> appropriate
> >> >> and
> >> >> >> >> never
> >> >> >> >> >>>> encountered a similar problem.
> >> >> >> >> >>>>
> >> >> >> >> >>>> I'd say WICKET-3171 is the rare
5% usecase. What's next,
> is
> >> >> >> overriding
> >> >> >> >> >>>> isEnabled() going to be declared
evil too? ;)
> >> >> >> >> >>>
> >> >> >> >> >>> yes
> >> >> >> >> >>>
> >> >> >> >> >>> -igor
> >> >> >> >> >>>
> >> >> >> >> >>>>
> >> >> >> >> >>>> Sven
> >> >> >> >> >>>>
> >> >> >> >> >>>> On Mon, 2010-11-29 at 11:22 -0600,
Douglas Ferguson wrote:
> >> >> >> >> >>>>
> >> >> >> >> >>>>> Can you explain why? We have
done this all over the
> place.
> >> >> >> >> >>>>>
> >> >> >> >> >>>>> D/
> >> >> >> >> >>>>>
> >> >> >> >> >>>>> On Nov 29, 2010, at 10:00 AM,
Martin Grigorov wrote:
> >> >> >> >> >>>>>
> >> >> >> >> >>>>> > The recommended way since
a few 1.4 releases is to
> >> override
> >> >> >> >> onConfigure()
> >> >> >> >> >>>>> > and call setVisible(true|false)
depending on your
> >> conditions.
> >> >> >> >> >>>>> >
> >> >> >> >> >>>>> > On Mon, Nov 29, 2010 at
4:49 PM, Douglas Ferguson <
> >> >> >> >> >>>>> > douglas@douglasferguson.us>
wrote:
> >> >> >> >> >>>>> >
> >> >> >> >> >>>>> >> Igor posted a comment
to this bug saying that
> overriding
> >> >> >> >> isVisible() is
> >> >> >> >> >>>>> >> "evil"
> >> >> >> >> >>>>> >>
> >> >> >> >> >>>>> >>
> https://issues.apache.org/jira/browse/WICKET-3171
> >> >> >> >> >>>>> >>
> >> >> >> >> >>>>> >> I was surprised by
this and am curious to hear more.
> >> >> >> >> >>>>> >>
> >> >> >> >> >>>>> >> D/
> >> >> >> >> >>>>>
> >> >> >> >> >>>>
> >> >> >> >> >>>>
> >> >> >> >> >>>>
> >> >> >> >> >>>
> >> >> >> >> >>
> >> >> >> >> >
> >> >> >> >>
> >> >> >> >
> >> >> >> >
> >> >> >> >
> >> >> >> > --
> >> >> >> > Pedro Henrique Oliveira dos Santos
> >> >> >> >
> >> >> >>
> >> >> >
> >> >> >
> >> >> >
> >> >> > --
> >> >> > Pedro Henrique Oliveira dos Santos
> >> >> >
> >> >>
> >> >
> >> >
> >> >
> >> > --
> >> > Pedro Henrique Oliveira dos Santos
> >> >
> >>
> >
> >
> >
> > --
> > Pedro Henrique Oliveira dos Santos
> >
>



-- 
Pedro Henrique Oliveira dos Santos

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message