[hibernate-dev] Envers compile issue

[Steve Ebersole]

In the meantime (to get the builds working again) I went ahead and 
implemented the explicit checking.  If you want to change something 
there, just look for usages of the new 
org.hibernate.envers.configuration.internal.metadata.FormulaNotSupportedException. 
 There are only 2 places it is used, both in MetadataTools...


On Fri 12 Apr 2013 08:03:59 AM CDT, Steve Ebersole wrote:
> Hi Lukasz,
>
> The problems seem limited to usages of:
> 1)
> org.hibernate.envers.configuration.internal.metadata.MetadataTools#getColumnNameIterator
> 2)
> org.hibernate.envers.configuration.internal.metadata.MetadataTools#addColumns
>
> from:
>
> 1)
> org.hibernate.envers.configuration.internal.metadata.AuditMetadataGenerator#createJoins
> 2)
> org.hibernate.envers.configuration.internal.metadata.BasicMetadataGenerator#addManyToOne
> 3)
> org.hibernate.envers.configuration.internal.metadata.BasicMetadataGenerator#addBasic
> 4)
> org.hibernate.envers.configuration.internal.metadata.CollectionMetadataGenerator#addWithMiddleTable
> 5)
> org.hibernate.envers.configuration.internal.metadata.CollectionMetadataGenerator#addValueToMiddleTable
> 6)
> org.hibernate.envers.configuration.internal.metadata.ToOneRelationMetadataGenerator#addToOne
>
>
> org.hibernate.envers.configuration.internal.metadata.MetadataTools#addColumns
> unfortunately goes through a number of internal addColumns/addColumn
> calls.
>
> Oddly enough, I found that there is in fact:
> 1)
> org.hibernate.envers.configuration.internal.metadata.MetadataTools#addColumnsOrFormulas
> 2)
> org.hibernate.envers.configuration.internal.metadata.MetadataTools#addFormula
>
> But that is only ever used to handle discriminators, as you
> mentioned.  Additionally, I then text searched the envers testsuite
> and found that only the
> org.hibernate.envers.test.integration.inheritance.single.discriminatorformula
> package contained any references to the text 'formula', which further
> illustrates that.
>
> So, all that said, I *think* envers already errored when it
> encountered formula (with CCE).  In the code I saw it is clearly
> expecting Iterator<Column> and not even checking if the Iterator
> returned Formula elements mixed in.
>
> At the end of the day it is still y'alls call.  Based on my reading of
> the current code, I think a comparable (though better) solution is to
> explicitly check each Iterator element type and throw that more
> descriptive exception[1].  That change I can make.  If y'all would
> rather add in real formula support, thats probably something y'all
> will have to work on.
>
> WDYT?
>
>
> [1] For example, FormulaNotSupportException( "Formula mappings are
> currently not supported in envers aside from @DiscriminatorValue; for
> more information, please see blah-blah-blah..." )
>
> On Apr 12, 2013 4:52 AM, "Łukasz Antoniak" <lukasz.antoniak at gmail.com
> <mailto:lukasz.antoniak at gmail.com>> wrote:
>
>     Hello Steve,
>
>     In general Envers does not support formula columns. Probably we
>     should save formula value to audit table. This can be tricky and I
>     have to think it over. Copying formula column to audit entity
>     would not produce expected result in all cases (e.g. time
>     functions used in formula expression). Currently we
>     support @DiscriminatorFormula annotation without saving its value
>     to audit table, since entities do not change their type.
>
>     @Adam: Thoughts? What do you think about copying formula column to
>     audit entity directly? Simplest solution AFAIK.
>
>     All usages of org.hibernate.mapping.Value#getColumnIterator(),
>     except the one in
>     org.hibernate.envers.configuration.internal.metadata.BasicMetadataGenerator,
>     map relation properties. IMO it is safe assumption that we are
>     dealing with columns there.
>
>     @Steve: if you agree, feel free to throw an exception if
>     encountered formula column in BasicMetadataGenerator asking to
>     register new feature.
>
>     Best regards,
>     Lukasz
>
>
>
>     2013/4/11 Steve Ebersole <steve at hibernate.org
>     <mailto:steve at hibernate.org>>
>
>         One option just came to mind.  Looking at the code, I have to
>         assume envers really just does not support formulas.
>          Otherwise, all these compile errors would eventually have
>         resulted in runtime (CCE) exceptions.
>
>         If that is really the case, I could add detection of that and
>         throw a more meaningful exception.  If that sounds like a
>         reasonable option, just let me know and I'll make that change.
>
>         Alternatively I could just log a message and keep going, but i
>         don't think that's a good choice.
>
>
>
>         On Thu 11 Apr 2013 04:07:27 PM CDT, Steve Ebersole wrote:
>
>             I just made a change to help catch runtime problems that
>             kept cropping
>             up.  The change was to
>             org.hibernate.mapping.Value#getColumnIterator.
>             The problem is that code in many modules (hem, envers)
>             that actually
>             deals with mapping code were making a bad assumption here.
>              The
>             returned iterator actually returns a Iterable<Selectable>, not
>             Iterable<Column>. Selectable is the contract shared
>             between Column and
>             Formula.  So when code non-defensively tries to treat that
>             thing as a
>             Iterable<Column> we often have issues.
>
>             Yes, the method is very poorly named.  Actually it
>             predates formula as
>             a feature.  But be that as it may, the code casting those
>             elements to
>             Column are just wrong.
>
>             For envers, I am not actually sure how to handle Formula
>             elements. I
>             need some help there.  The issues are all isolated to
>             org.hibernate.envers.configuration.internal.metadata
>              Could someone
>             more familiar with envers (and especially its entity
>             definition stuff)
>             take a look?
>
>             Thanks!
>
>