[hibernate-issues] [Hibernate-JIRA] Commented: (HV-441) Refactor BeanMetaDataImpl

[Hardy Ferentschik (JIRA)]

    [ http://opensource.atlassian.com/projects/hibernate/browse/HV-441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=40042#action_40042 ] 

Hardy Ferentschik commented on HV-441:
--------------------------------------

bq. When implementing method validation I found that the meta model was not expressive enough for the new method stuff, which was why I came up with more powerful model classes accompanying BeanMetaData such as MethodMetaData and ParameterMetaData.

Sure, I got that and I understand we needed more meta information for method level information. We just have to make sure that we know where we are heading now and that we will pull things together again at some stage. {{BeanMetaDataImpl}} is admittedly complex, but at least it bundled all information in one place. I am most worried about other people trying to make changes to the code w/o knowing the dependencies.  


bq. The idea was that the engine (ValidatorImpl) could simply ask a MethodMetaData whether it requires cascaded validation or not for instance. At the same time I tried to to modify existing functionality in BeanMetaDataImpl not more than necessary.

If this is the paths we are taking ({{MethodMetaData}}) for fields and everything else.

bq. So currently the situation is, that method validation is based on MethodMetaData#isCascading(), while standard bean/property validation relies on BeanMetaDataImpl#cascadingMembers. In BMDI's constructor both structures are maintained.

I guess for now we have to live with that, but this should be addresses asap.

bq. So I definitely think we should get these things more aligned again but I've got the feeling we can live with the situation temporarily. WDYT?

Sure. When it comes to the actual refactoring I like your idea of separating the construction process (annotation processing) from the actual model encapsulated in {{BeanMetaData}}. I think you suggested some sort of builder pattern. The current {{BeanMetaDataImpl}} is getting ways to big.

> Refactor BeanMetaDataImpl
> -------------------------
>
>                 Key: HV-441
>                 URL: http://opensource.atlassian.com/projects/hibernate/browse/HV-441
>             Project: Hibernate Validator
>          Issue Type: Task
>          Components: engine
>            Reporter: Hardy Ferentschik
>             Fix For: 4.x
>
>
> Due to the introduction of method level validation the {{BeanMetaDataImpl}} abstraction started leaking. For example parts of the configuration data is now held at multiple places. We have to start addressing this and pull things together again.
> I started a discussion around this with Gunnar. My original question was:
> {quote}
> Could you have a look at my branch https://github.com/hferentschik/hibernate-validator/commits/HV-433. The tests for this issue are passing (CascadingWithConstraintMappingTest), in fact the whole test suite is passing. The problem I encountered was around MethodMetaData and its cascading flag. In the case of the test case it was not properly set. The code was just looking for the @Valid annotation, but also has to check whether the method is explicitly (xml or programmatic api) configured for cascading.
> If you have a look at the branch you see how I changed that. The question which came up in my mind is, whether there is not duplication between MethodMetaData and BeanMetaData (at least regarding the cascading).
> {quote}
> Gunnar's response:
> {quote}
> Yes, that's certainly right. Actually I'm not quite happy with BeanMetaDataImpl altogether atm. It is pretty complex code, is quite huge and I think we should tidy up/restructure it for 4.3. 
> When implementing method validation I found that the meta model was not expressive enough for the new method stuff, which was why I came up with more powerful model classes accompanying BeanMetaData such as MethodMetaData and ParameterMetaData. The idea was that the engine (ValidatorImpl) could simply ask a MethodMetaData whether it requires cascaded validation or not for instance. At the same time I tried to to modify existing functionality in BeanMetaDataImpl not more than necessary. So currently the situation is, that method validation is based on MethodMetaData#isCascading(), while standard bean/property validation relies on BeanMetaDataImpl#cascadingMembers. In BMDI's constructor both structures are maintained. 
> Actually I'm proceeding on that path for HV-371. There I introduced AggregatedMethodMetaData which represents a given method *and* all the methods up in the hierarchy which it overrides/implements (basically this replaces the Map<Class<?>, Map<Method, MethodMetaData>> methodMetaConstraints with Map<Method, AggregatedMethodMetaData> methodMetaData. This should make things much easier in ValidatorImpl, which now can deal with one AggregatedMethodMetaData object instead of iterating over the complete inheritance hierarchy (So for instance AMMD#isCascading() answers whether a cascaded validation is required for the given method no matter where in the hierarchy it was marked with @Valid).
> So I definitely think we should get these things more aligned again but I've got the feeling we can live with the situation temporarily. WDYT?
> {quote}

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: http://opensource.atlassian.com/projects/hibernate/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira