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

[Gunnar Morling (JIRA)]

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

Gunnar Morling commented on HV-441:
-----------------------------------

Yes, we should double-check all changes to {{BeanMetaDataImpl}}, the pull request workflow on GitHub seems really valuable here. I think in the end things will be much easier to understand, with clearly focused classes.

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

I'm not sure I understand. Do you mean we should have a similar type for fields ({{FieldMetaData}})?

My idea for the general approach is that we should have rich model classes which each encapsulate one single concept/entity. An example is [AggregatedMethodMetaData|https://github.com/gunnarmorling/hibernate-validator/blob/HV-371/hibernate-validator/src/main/java/org/hibernate/validator/metadata/AggregatedMethodMetaData.java] which represents one given method and all the super type methods which it overrides/implements. It provides simple to use functionality such as {{isCascaded()}} etc. which encapsulates the complexity of the hierarchy behind. This makes things much easier for {{ValidatorImpl}} for instance, which simple can ask the model whether a cascaded validation is required. Of course any redundancies should be removed by putting each functionality to the appropriate model class.

As you write the creation process for the model should be separated from the model itself. So ideally all the annotation retrieval stuff should be removed from BeanMetaDataImpl to a dedicated builder class dealing with these things. There might be three distinct model builders (for annotation-based models, the programmatic configuration API and XML).

WDYT, for when should we approach these issues? 4.2, or later? I feel 4.2 would make pretty much sense, but of course that means that we'll need some more time for this release.

> 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