[hibernate-dev] Pull request for HHH-2394 (Support filter tag in subclass)

[Steve Ebersole]

1) I think adding support for {alias} is a more general issue.  Its 
applicable to many other pieces of mapping metadata (@Formula, etc).  
More I was responding to David's comment there.  I totally think it 
makes sense to support this in all of those cases.  I guess the vote 
point there is whether we want to introduce that support piecemeal or 
develop it across the board.  If we do do it piecemeal I think another 
piece of information we will need is a flag indicating whether to scan 
for alias injection points.  By "scanning" I mean the processing we do 
today where we tokenize the fragment string and then try to guess where 
an alias is needed.   Let's call this flag 'deduceAliasInjectionPoints' 
for now.  I tend to favor meaningful attribute names so my suggested 
names tend to get a little long; I am open to suggestions for a more 
succinct name.  Anyway, I think it needs to be TRUE by default to 
maintain backwards compatible behavior.  Here is an example:

@Filter(name="iqMin", table="ZOOLOGY_HUMAN", 
condition="{alias}.HUMAN_IQ >= :min", deduceAliasInjectionPoints=false)

Here there is no need to deduce the points at which to inject an alias; 
we have done that for Hibernate in the supplied condition fragment.

I think another discussion revolves around cases in which the fragment 
involves references to columns from multiple tables.  Mostly this is 
important for scenarios involving secondary tables and inheritance.  Do 
we want to provide support for that?  Using the classic 
org.hibernate.test.hql.Animal hierarchy from the testsuite, maybe 
something like:

@Filter(
    name="iqMin",
    condition="{a}.bodyWeight >= :minWeight and {m}.birthdate < 
:ageCutOffBirthDate",
    deduceAliasInjectionPoints=false,
    aliases={
        @SqlFragmentAlias( alias="a", entity=Animal.class ),
        @SqlFragmentAlias( alias="m", entity=Mammal.class )
    }
)

NOTE : @SqlFragmentAlias would have support for both 'entity' (as 
convenience form) and 'table' attributes.


2) Just wanted to make sure it was clear that I meant that *if* we add 
support for 'entity' that it would be an alternative option to 'table' 
*in some cases*.  There would be times (secondary tables) when 'entity' 
would not be useful.  Also, I am not overly fond of the term 'entity'.  
Its vague here IMHO.  I think the attribute name ought to describe 
"why".  (Yes the term 'entity' is used elsewhere in annotations, but I 
think the use there is more meaningful).  The thing we are trying to 
express here is the "entity hierarchy class whose table is being 
referenced".  Is that something that is OK to delineate just in 
javadoc?  Dunno.  What does everyone else think?  Am I just being too 
anal about using the term 'entity' here?


On Mon 04 Jun 2012 10:07:52 AM CDT, Rob Worsnop wrote:
> Steve,
> Thanks for the feedback.
>
> Is this a correct summary:
>
> * Introduce the "{alias}" placeholder, as in Restrictions.sqlRestriction.
> * Introduce another optional @Filter element, "entity", which
> developers could use instead of the proposed "table" element.
>
> Thanks,
> Rob.
>
>
> On Fri, Jun 1, 2012 at 12:56 PM, Steve Ebersole<steve at hibernate.org>  wrote:
>> Hi Rob,   overall I am OK with the proposal.
>>
>> Some comments inline
>>
>> On Thu 31 May 2012 03:32:25 PM CDT, David Mansfield wrote:
>>>> It seemed that the challenge to overcome was that Hibernate did not
>>>> know which aliases to use when generating the where clauses for
>>>> filters. When filters are not allowed on subclasses (current
>>>> situation), the alias for the root entity is used for all where
>>>> clauses. So when FilterHelper is asked to render the filters, it does
>>>> so with a single alias string.
>>>
>>>> So my changes revolve around two themes:
>>>>
>>>> 1. Remembering the filter's associated table so that we can generate
>>>> an appropriate alias. This was done by replacing the name->condition
>>>> map with a list of FilterConfiguration objects. This is a new class
>>>> and includes a qualified table name (as well as the name and
>>>> condition, of course).
>>>>
>>>> 2. Passing a callback (instead of the alias string) to
>>>> FilterHelper.render() that allows it to generate an appropriate alias
>>>> for each filter, depending on table name. The callback is an
>>>> implementation of a new interface, FilterAliasGenerator. The
>>>> implementation varies by context.
>>> The same problem exists in Criteria queries (HHH-1161)  when using the
>>> sqlRestriction.  In that use case, one uses the string '{alias}' as a
>>> placeholder in the SQL, and the engine replaces it, however as with your
>>> problem, it always replaces with the subclass alias, and therefore
>>> cannot be used to create a restriction on any superclass properties.
>>>
>>> It seems a more generic approach to the '{alias}' syntax which allows
>>> for, perhaps, '{entityName.alias}' (e.g.
>>> '{com.example.Mammal.alias}.is_pregnant = 1' would work, then your
>>> filter conditions wouldn't need the "table" attribute, and the fix would
>>> possibly be extensible for HHH-1161.
>>
>> To be honest, I still think an embedded hint for the alias placement is
>> a good idea.  There is a lot of chicanery in Hibernate code to try to
>> figure out the correct place to put aliases in sql fragments.
>> Something like this removes that ambiguity:
>>
>> @OneToMany(mappedBy="club")
>> @Filters({
>>     @Filter(name="iqMin", table="ZOOLOGY_HUMAN",
>> condition="{alias}.HUMAN_IQ>= :min"),
>>     @Filter(name="pregnantMembers", table="ZOOLOGY_MAMMAL",
>> condition="{alias}.IS_PREGNANT=1")
>> })
>> private Set<Human>  members = new HashSet<Human>();
>>
>> Its really 2 different concerns.
>>
>>
>>> Alternatively, there are already some annotations which take an entity
>>> directly (such as @ManyToOne), as a class reference, which is way more
>>> typesafe and refactoring friendly.  So instead of
>>> "table='ZOOLOGY_MAMMAL'" you could have "entity=Mammal.class".  This
>>> would require more bookeeping on the implementation side, but would be a
>>> superior abstraction IMHO.
>>
>> It would need to allow a mix of both I think.  The Class reference is a
>> good alternative for inheritance as initially discussed here.  But
>> another place this comes up is in regards to SecondaryTables, and there
>> we would need to know the table name.
>>
>>
>> --
>> steve at hibernate.org
>> http://hibernate.org
>> _______________________________________________
>> hibernate-dev mailing list
>> hibernate-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev

--
steve at hibernate.org
http://hibernate.org