search

jakartaee / validation-tck

Jakarta Validation TCK
http://validator.hibernate.org
Apache License 2.0
38 stars 33 forks source link

Add support for CDI 4 #184

Closed jasondlee closed 2 years ago

jasondlee commented 2 years ago

CDI 4 changes how empty beans.xml is interpreted. This change adds a new method to add a beans.xml that preserves CDI 3 behavior. The resulting tests will behave the same under CDI 3 and 4.

jasondlee commented 2 years ago

CDI-wise this change is correct under the assumption that the TCK is always executed with CDI Full (i.e. with something like EE server behind it; it wouldn't work with CDI Lite). I didn't check for completeness of changes but I guess you verified it with WFLY :)

Yes, this does run with WildFly. As to the CDI version issue, I guess that would have to be the BV team's call. If the TCK is targeting EE 10, the CDI 4 seems right, but since EE 10 doesn't have any new features from BV, perhaps CDI 3 is more appropriate. For now, though, I'll leave this as it is unless changes are requested.

manovotn commented 2 years ago

Yes, this does run with WildFly. As to the CDI version issue, I guess that would have to be the BV team's call. If the TCK is targeting EE 10, the CDI 4 seems right, but since EE 10 doesn't have any new features from BV, perhaps CDI 3 is more appropriate

That is not what I meant - CDI 4 (for EE 10) has split into Lite and Full profiles. And while EE should stay Full (i.e. what it was now - all functionalities), some specs might want to target Lite instead which is a subset of features supported by Full and allowing CDI to run in more restricted environments such as build time. In order to support Lite, these TCKs would need to change to avoid using discovery mode all. But this PR is good the way it is, it can always be adjusted later if needed.

gsmet commented 2 years ago

So I have nothing against this change. What I'm wondering is if it's a good idea to include it in a micro or if we should release a 3.1.0 targeting EE 10 instead (even if the spec hasn''t changed). My main concern is that it could break a CDI implementation somewhere.

I checked with HV 8 and WildFly preview (with a version that still is using CDI < 4) and it seems to work OK in the Weld case but I wouldn't guarantee it will work with all of the EE 9 CDI impl out there.

@starksm64 WDYT?

manovotn commented 2 years ago

My main concern is that it could break a CDI implementation somewhere.

@gsmet Prior to CDI 4, empty beans.xml == beans.xml with discovery mode element equal to all so the point of this change is that it makes the TCK CDI version agnostic.

However, your comment made me think that all these beans.xml files are generated with schema location and version for CDI 4 and some very strict impls might in theory have issues with that - not that I'd know of any. Even CDI TCK itself has had and still has beans.xml files in it with ancient versions and no impl complained so far. Take a look at this one for instance, that's as old as they get :)

gsmet commented 2 years ago

Even CDI TCK itself has had and still has beans.xml files in it with ancient versions and no impl complained so far.

I don't expect issues with newer implementations and old beans.xml files but I could foresee issues with older implementations and newer beans.xml.

Maybe we should use a EE 9 compatible descriptor given it's the lowest version we want to support? I think it would be safer.

jasondlee commented 2 years ago

Maybe we should use a EE 9 compatible descriptor given it's the lowest version we want to support? I think it would be safer.

I'll update the PR with an EE 9 descriptor.

jasondlee commented 2 years ago

In case you missed the push, this now has an EE 9 beans.xml.

gsmet commented 2 years ago

I haven't missed it :). I tested it with HV 8 and WildFly with EE 9 and it works OK.

@starksm64 I think we should merge it and release a micro of the TCK.

jasondlee commented 2 years ago

No worries. I just realized I never confirmed that I did indeed do what I said I was going to, and if the push to the branch wasn't noisy enough, you could still be waiting. :) At any rate, thanks for the attention on this. I think once this is merged and published, we can show WildFly 27 as spec compliant on this. :)

starksm64 commented 2 years ago

I have tried releasing this, but the EF CI is complaining about being able to access https://repository.jboss.org/nexus/content/groups/public-jboss/ due to 503 errors. Looking into the problem.

gsmet commented 2 years ago

@starksm64 yeah JBoss Nexus has issues since the migration. But I'm surprised we are dependent on it, we shouldn't be.

gsmet commented 2 years ago

I'll check if we can get rid of it.

starksm64 commented 2 years ago

Nexus works from my local machine. It is being used for the audit report.

starksm64 commented 2 years ago 
Caused by: org.apache.maven.wagon.TransferFailedException: transfer failed for https://repository.jboss.org/nexus/content/groups/public-jboss/org/jboss/test-audit/jboss-test-audit-impl/1.1.3.Final/jboss-test-audit-impl-1.1.3.Final.pom, status: 503 Service Unavailable
    at org.apache.maven.wagon.providers.http.wagon.shared.AbstractHttpClientWagon.fillInputData (AbstractHttpClientWagon.java:1204)
    at org.apache.maven.wagon.providers.http.wagon.shared.AbstractHttpClientWagon.fillInputData (AbstractHttpClientWagon.java:1140)
    at org.apache.maven.wagon.StreamWagon.getInputStream (StreamWagon.java:126)
    at org.apache.maven.wagon.StreamWagon.getIfNewer (StreamWagon.java:88)
    at org.apache.maven.wagon.StreamWagon.get (StreamWagon.java:61)

but locally:

└> wget https://repository.jboss.org/nexus/content/groups/public-jboss/org/jboss/test-audit/jboss-test-audit-impl/1.1.3.Final/jboss-test-audit-impl-1.1.3.Final.pom
--2022-05-25 11:22:02--  https://repository.jboss.org/nexus/content/groups/public-jboss/org/jboss/test-audit/jboss-test-audit-impl/1.1.3.Final/jboss-test-audit-impl-1.1.3.Final.pom
Resolving repository.jboss.org (repository.jboss.org)... 2600:141c:f000::b857:c50a, 2600:141c:f000::b857:c510, 23.200.36.147, ...
Connecting to repository.jboss.org (repository.jboss.org)|2600:141c:f000::b857:c50a|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 2952 (2.9K) [application/xml]
Saving to: ‘jboss-test-audit-impl-1.1.3.Final.pom’

jboss-test-audit-im 100%[===================>]   2.88K  --.-KB/s    in 0s      

2022-05-25 11:22:02 (22.0 MB/s) - ‘jboss-test-audit-impl-1.1.3.Final.pom’ saved [2952/2952]
gsmet commented 2 years ago

Yeah I was checking too: this one is not available on Maven Central unfortunately. I wonder if we could make it happen?

For JBoss Nexus, they have very weird networking issues since the migration to the new datacenter, they are working on it AFAIK. I have the same issue with GH Actions and got rid of everything that was still using JBoss Nexus in Quarkus.

starksm64 commented 2 years ago

Ok, perhaps we can just create a temp branch and remove the audit report stuff so I can build the release from that.

gsmet commented 2 years ago

Maybe let's wait until Monday to see if they can get JBoss Nexus fixed? And then we decide what to do.

starksm64 commented 2 years ago

Fine with me.

starksm64 commented 2 years ago

I have added a workaround of installing the repository.jboss.org artifacts used by the tck build in the Jenkins CI job, so I have been able to release a 3.0.1 TCK to staging: https://download.eclipse.org/ee4j/bean-validation/3.0/beanvalidation-tck-dist-3.0.1.zip

@jasondlee, can someone from WildFly team verify that before I promote it?

jasondlee commented 2 years ago

@starksm64 Working on this now.

starksm64 commented 2 years ago

This has been promoted: https://download.eclipse.org/jakartaee/bean-validation/3.0/beanvalidation-tck-dist-3.0.1.zip