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@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
