4.3.2.final patch #330

Merged
merged 7 commits into from Jul 24, 2014

3 participants

@hferentschik
Hibernate member

For 4.3.2 I only applied the HV-912 changes. I did not move ConstraintMapping as on master (part of the "reducing accessibility of some classes and methhods". I think it is technically not necessary and it could break existing clients. @gunnarmorling, wdyt?

@hferentschik
Hibernate member

@gunnarmorling any chance you can run this version with a security manager enabled?

@cloudbees-pull-request-builder

HV-5-MASTER #514 FAILURE
Looks like there's a problem with this pull request

@gunnarmorling gunnarmorling commented on an outdated diff Jul 24, 2014
...nal/cfg/context/ConstraintMappingContextImplBase.java
@@ -87,4 +90,63 @@ public MethodConstraintMappingContext method(String name, Class<?>... parameterT
return new MethodConstraintMappingContextImpl( beanClass, method, mapping );
}
+
+ /**
+ * Returns the member with the given name and type.
+ *
+ * @param clazz The class from which to retrieve the member. Cannot be {@code null}.
+ * @param property The property name without 'is', 'get' or 'has'. Cannot be {@code null} or empty.
+ * @param elementType The element type. Either {@code ElementType.FIELD} or {@code ElementType METHOD}.
+ *
+ * @return the member which matching the name and type or {@code null} if no such member exists.
+ */
+ public static Member getMember(Class<?> clazz, String property, ElementType elementType) {
@gunnarmorling
Hibernate member
gunnarmorling added a line comment Jul 24, 2014

This should not be public.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gunnarmorling gunnarmorling and 1 other commented on an outdated diff Jul 24, 2014
...alidator/internal/metadata/raw/ConstrainedMethod.java
@@ -113,7 +116,7 @@ public ConstrainedMethod(
this.hasParameterConstraints = hasParameterConstraints( parameterMetaData );
if ( isConstrained() ) {
@gunnarmorling
Hibernate member
gunnarmorling added a line comment Jul 24, 2014

The method should not be set accessible here, only the copy hold in ValidatorImpl.

@hferentschik
Hibernate member
hferentschik added a line comment Jul 24, 2014

Of course. I missed this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gunnarmorling
Hibernate member

I think it is technically not necessary and it could break existing clients.

Makes sense. I had moved it to reduce some visibilities but we cannot do it in 4.3 as we don't have the IF/class split for it there.

@gunnarmorling gunnarmorling and 1 other commented on an outdated diff Jul 24, 2014
...validator/internal/metadata/raw/ConstrainedField.java
}
public BeanConstraintLocation getLocation() {
return (BeanConstraintLocation) super.getLocation();
}
+ @Override
@gunnarmorling
Hibernate member
gunnarmorling added a line comment Jul 24, 2014

Interesting, was this part of the original change? Not that it does harm, just wondering.

@hferentschik
Hibernate member
hferentschik added a line comment Jul 24, 2014

it was directly added by your changes, but the version on 4.3 did not have an equals at all. I thought it would not harm. On the other hand hashCode is missing, so I probably just get rid of it altogether.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gunnarmorling gunnarmorling commented on the diff Jul 24, 2014
...g/hibernate/validator/internal/util/StringHelper.java
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+package org.hibernate.validator.internal.util;
+
+import java.util.Arrays;
+
+/**
+ * Helper class dealing with strings.
+ *
+ * @author Gunnar Morling
+ */
+public class StringHelper {
@gunnarmorling
Hibernate member
gunnarmorling added a line comment Jul 24, 2014

Oh, how did this become part of the back-port? I don't remember to have touched it in the original PR.

@hferentschik
Hibernate member
hferentschik added a line comment Jul 24, 2014

You didn't. During the merge of ReflectionHelper I had to choose the version of getPropertyName to use. I went for the one using the StringHelper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gunnarmorling gunnarmorling and 1 other commented on an outdated diff Jul 24, 2014
.../internal/util/annotationfactory/AnnotationProxy.java
@@ -130,4 +133,46 @@ public String toString() {
result.addAll( values.keySet() );
return result;
}
+
+ private boolean areEqual(Object o1, Object o2) {
@gunnarmorling
Hibernate member
gunnarmorling added a line comment Jul 24, 2014

Hum, same here. Apparently some more changes sneaked into the back-port.

@hferentschik
Hibernate member
hferentschik added a line comment Jul 24, 2014

this method is actually not used. I remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gunnarmorling
Hibernate member

@gunnarmorling any chance you can run this version with a security manager enabled?

Yes, I'll give it a try.

@gunnarmorling
Hibernate member

I'm seeing one permission issue, but I'm not totally clear about the cause. ValidationXmlParser cannot access the XML mapping files in the TCK JAR. I need to assign the following permission to the HV code base:

permission java.io.FilePermission "/path/to/jsr303-tck-1.0.6.GA.jar", "read";

That's not required on master. I'm wondering whether it's related to that special classloader which is used in the 1.0 TCK. Does this ring a bell in any way?

@hferentschik
Hibernate member

Pushed a forced update to apply review comments

@cloudbees-pull-request-builder

HV-5-MASTER #517 FAILURE
Looks like there's a problem with this pull request

@gunnarmorling
Hibernate member

One more test is failing when running with a security manager, ConstraintDefinitionsTest. This is due to annotation methods not being set to accessible before invoking them which has been fixed upstream with HV-843. For the sake of completeness, we should backport this fix I guess. Relevance in practice is rather low as it only is a problem for package-private constraint types, so we could also ignore that one test failure. WDYT?

@hferentschik
Hibernate member

That's not required on master. I'm wondering whether it's related to that special classloader which is used in the 1.0 TCK. Does this ring a bell in any way?

Not really. Obviously the harness is executed differently. On the 4.3 branch is was something homegrown and on master it is Arquillian. How do you run the tests? In container or not?

@hferentschik
Hibernate member

One more test is failing when running with a security manager, ConstraintDefinitionsTest. This is due to annotation methods not being set to accessible before invoking them which has been fixed upstream with HV-843.

I remember vaguely.

For the sake of completeness, we should backport this fix I guess.

I see how hard it is to apply.

Relevance in practice is rather low as it only is a problem for package-private constraint types

Sure. Let me have a quick look.

@hferentschik
Hibernate member

I added a commit with the HV-843 backport. @gunnarmorling, could you check again?

@cloudbees-pull-request-builder

HV-5-MASTER #518 FAILURE
Looks like there's a problem with this pull request

@gunnarmorling
Hibernate member

@hferentschik, Ah you pushed another update with the JAXB fix already, right?

@gunnarmorling gunnarmorling merged commit 763feff into hibernate:4.3 Jul 24, 2014
@hferentschik
Hibernate member

Ah you pushed another update with the JAXB fix already, right?

yes

@cloudbees-pull-request-builder

HV-5-MASTER #521 FAILURE
Looks like there's a problem with this pull request

@hferentschik
Hibernate member

thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment