5:10 a.m.
Hi,
= Context =
So, my patch here [1] broke a test which checks that Criteria +
restrictions mostly work - even if it's documented as not supported
and not working.
"Mostly" as in "you can't get the result size but you might get the
results". See [2] for explanations.
I spent some time yesterday contemplating this issue and, while I'm
sorry for breaking this test, I still think we should apply my patch,
remove this test and make this case not supported for good.
= Why it mostly works =
In the original ObjectLoaderHelper implementation, we use
session.load: it doesn't force the proxy to be initialized. If a proxy
for an entity isn't initialized, it's filtered out from the results.
It's the job of the various implementations of ObjectsInitializer to
initialize the objects in the session so that they are later included
in the results.
In the case of Criteria + restrictions, the restrictions are applied
in the ObjectsInitializer so the entity which doesn't satisfy the
restrictions are not initialized in the ObjectsInitializer... and thus
not included in the results.
= Why my patch is breaking this behaviour consistently =
In my patch, I use Session.get which forces the initialization of the
proxy and I removed the filter removing the uninitialized proxies: it
became unnecessary as I was sure all proxies are now initialized.
This patch has been designed to solve HSEARCH-1448 and to simplify the
ObjectLoaderHelper code which was quite complicated.
Situation after my patch: all the results satisfying the full text
search are returned. The restrictions of the criteria are not taken
into account. In fact, it works as documented.
= Relying on the session state to filter out entities is wrong =
So the fact is that we basically rely on the session state to filter
out the results we don't want.
I had to check that my gut feeling was right so I checked out current
master, opened ResultSizeOnCriteriaTest and just added the following
lines before the search call:
//load in the session the object which shouldn't be returned just for fun
session.get( Tractor.class, 2 );
-> the object is returned and the test is broken too. This is expected
behaviour as this object has been initialized in the session and is
now considered as a valid candidate for the results.
= Conclusion =
I don't think we can have a really working criteria + restrictions
thing without refactoring a lot of things in search: the Initializer +
Loader concept can't work reliably in this case.
Therefore I think we should simply remove this test and clearly make
it fail as it can be a potential security flaw if we return entities
the user shouldn't see just because they were initialized in the
session for another purpose.
We might revisit it later but I really think it's a lot of work to get it right.
Thoughts?
References
[1] https://github.com/hibernate/hibernate-search/pull/581
[2] https://hibernate.atlassian.net/browse/HSEARCH-753
6:14 a.m.
Hi Guillaume,
nice job with this issue. I was about to comment on the pull request, but
now I do it here.
On 21 Jan 2014, at 11:10, Guillaume Smet <guillaume.smet(a)hibernate.org> wrote:
So, my patch here [1] broke a test which checks that Criteria +
restrictions mostly work - even if it's documented as not supported
and not working.
"Mostly" as in "you can't get the result size but you might get the
results". See [2] for explanations.
I spent some time yesterday contemplating this issue and, while I'm
sorry for breaking this test, I still think we should apply my patch,
remove this test and make this case not supported for good
I think you are right. I also would simplify the code and remove this test.
The documentation is clear about this:
"Only fetch mode can be adjusted, refrain from applying any other restriction.”
I am even wondering whether we should not go a step further and disallow
the use of Criterias altogether in FullTextSessionImpl by throwing an exception
in case of its use. We then can add an explicit method to FullTextSession which
allows just the setting of the fetch type for a given association of an entity.
Under the hood it can still use the Criteria, but it will prevent the unsupported use
of Criteria queries.
+1 for applying the patch and removing the test
and I probably would even go a step further.
—Hardy
6:50 a.m.
Hi Hardy,
On Fri, Mar 21, 2014 at 12:14 PM, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
I am even wondering whether we should not go a step further and
disallow
the use of Criterias altogether in FullTextSessionImpl by throwing an exception
in case of its use. We then can add an explicit method to FullTextSession which
allows just the setting of the fetch type for a given association of an entity.
Under the hood it can still use the Criteria, but it will prevent the unsupported use
of Criteria queries.
Thanks for your feedback.
As for me, I think generalizing what is done in
FullTextQueryImpl.getResultSize() when isSizeSafe() is false might be
sufficient for the moment.
I don't use this option though so I don't know exactly why it's used
and if being able to set a Criteria query might be useful.
--
Guillaume
7:41 a.m.
The question of being able to filter via Criteria or Hibernate Session
Filters is a recurring feature request so we need to be cautious here.
You make good points, and if it comes to a choice of support either/or
we'll certainly drop the Criteria filtering as obviously we won't make
concessions on correctness.
I'm sure that Criteria filtering is actually being used by several
other teams, and while they are aware of it being "borderline", it's
good enough for them; so rather than drop it I want to have a better
look at alternatives; worst case I'll end up with a refreshed
understanding of this code area.
Ultimately if we decide to drop it altogether, we might want to remove
the option of specifying the Criteria altogether in favor or a "fetch
graph" hint from JPA 2.1: probably more suited for the purpose and
gives no room for unpredictable behaviour.
-- Sanne
On 21 March 2014 11:50, Guillaume Smet <guillaume.smet(a)gmail.com> wrote:
Hi Hardy,
On Fri, Mar 21, 2014 at 12:14 PM, Hardy Ferentschik <hardy(a)hibernate.org> wrote:
> I am even wondering whether we should not go a step further and disallow
> the use of Criterias altogether in FullTextSessionImpl by throwing an exception
> in case of its use. We then can add an explicit method to FullTextSession which
> allows just the setting of the fetch type for a given association of an entity.
> Under the hood it can still use the Criteria, but it will prevent the unsupported
use
> of Criteria queries.
Thanks for your feedback.
As for me, I think generalizing what is done in
FullTextQueryImpl.getResultSize() when isSizeSafe() is false might be
sufficient for the moment.
I don't use this option though so I don't know exactly why it's used
and if being able to set a Criteria query might be useful.
--
Guillaume
_______________________________________________
hibernate-dev mailing list
hibernate-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
7:58 a.m.
On 21 Jan 2014, at 13:41, Sanne Grinovero <sanne(a)hibernate.org> wrote:
The question of being able to filter via Criteria or Hibernate
Session
Filters is a recurring feature request so we need to be cautious here.
The question is can we support it properly (most notably correctly).
You make good points, and if it comes to a choice of support
either/or
we'll certainly drop the Criteria filtering as obviously we won't make
concessions on correctness.
I'm sure that Criteria filtering is actually being used by several
other teams, and while they are aware of it being "borderline", it's
good enough for them; so rather than drop it I want to have a better
look at alternatives; worst case I'll end up with a refreshed
understanding of this code area.
sure, another look cannot hurt.
Ultimately if we decide to drop it altogether, we might want to
remove
the option of specifying the Criteria altogether in favor or a "fetch
graph" hint from JPA 2.1: probably more suited for the purpose and
gives no room for unpredictable behaviour.
Good idea. Maybe I was too extreme with suggesting to throw an exception
when using Criteria, but adding a decocted and predictable way for specifying
fetch behaviour seems like the way forward. If nothing else we can then remove
the mention of Criteria from the documentation in favour of the new dedicated API.
If then someone wants to use Criteria he really does it on its on risk.
—Hardy
9:01 a.m.
I don't ahve much to say besides what has been said on this thread
already.
The "tolerated" feature works in less cases than I anticipated as
Guillaume demonstrated. The different with the new patch is that it
happens systematically (on criteria restrictions) and leads to a lot of
unitary entity loads. In other words, it wronger and slower :)
A proper implementation of this feature, would require a redesign of the
query SPIs I suppose.
Emmanuel
On Fri 2014-03-21 11:10, Guillaume Smet wrote:
Hi,
= Context =
So, my patch here [1] broke a test which checks that Criteria +
restrictions mostly work - even if it's documented as not supported
and not working.
"Mostly" as in "you can't get the result size but you might get the
results". See [2] for explanations.
I spent some time yesterday contemplating this issue and, while I'm
sorry for breaking this test, I still think we should apply my patch,
remove this test and make this case not supported for good.
= Why it mostly works =
In the original ObjectLoaderHelper implementation, we use
session.load: it doesn't force the proxy to be initialized. If a proxy
for an entity isn't initialized, it's filtered out from the results.
It's the job of the various implementations of ObjectsInitializer to
initialize the objects in the session so that they are later included
in the results.
In the case of Criteria + restrictions, the restrictions are applied
in the ObjectsInitializer so the entity which doesn't satisfy the
restrictions are not initialized in the ObjectsInitializer... and thus
not included in the results.
= Why my patch is breaking this behaviour consistently =
In my patch, I use Session.get which forces the initialization of the
proxy and I removed the filter removing the uninitialized proxies: it
became unnecessary as I was sure all proxies are now initialized.
This patch has been designed to solve HSEARCH-1448 and to simplify the
ObjectLoaderHelper code which was quite complicated.
Situation after my patch: all the results satisfying the full text
search are returned. The restrictions of the criteria are not taken
into account. In fact, it works as documented.
= Relying on the session state to filter out entities is wrong =
So the fact is that we basically rely on the session state to filter
out the results we don't want.
I had to check that my gut feeling was right so I checked out current
master, opened ResultSizeOnCriteriaTest and just added the following
lines before the search call:
//load in the session the object which shouldn't be returned just for fun
session.get( Tractor.class, 2 );
-> the object is returned and the test is broken too. This is expected
behaviour as this object has been initialized in the session and is
now considered as a valid candidate for the results.
= Conclusion =
I don't think we can have a really working criteria + restrictions
thing without refactoring a lot of things in search: the Initializer +
Loader concept can't work reliably in this case.
Therefore I think we should simply remove this test and clearly make
it fail as it can be a potential security flaw if we return entities
the user shouldn't see just because they were initialized in the
session for another purpose.
We might revisit it later but I really think it's a lot of work to get it right.
Thoughts?
References
[1] https://github.com/hibernate/hibernate-search/pull/581
[2] https://hibernate.atlassian.net/browse/HSEARCH-753
_______________________________________________
hibernate-dev mailing list
hibernate-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
3933
days inactive
3933
days old
5 comments
5 participants
participants (5)
-
Emmanuel Bernard -
Guillaume Smet -
Guillaume Smet -
Hardy Ferentschik -
Sanne Grinovero