Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove injection and lookups for raw maps according to challenge #1710 #1723

Merged
merged 3 commits into from
Nov 23, 2022
Merged

Remove injection and lookups for raw maps according to challenge #1710 #1723

merged 3 commits into from
Nov 23, 2022

Conversation

arjantijms
Copy link
Contributor

Fixes #1710

Signed-off-by: Arjan Tijms [email protected]

@arjantijms arjantijms added the TCK Any issue having to do with the TCK not the API of SPEC text label Nov 15, 2022
@arjantijms arjantijms added this to the 4.0.1 milestone Nov 15, 2022
@arjantijms arjantijms requested a review from brideck November 15, 2022 15:45
Copy link
Contributor

@brideck brideck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should also remove/disable the corresponding tests:

  • faces23 - Spec1323IT (ApplicationMap)
  • faces23 - Spec1335IT (ViewMap)
  • faces23 - Spec1345IT (SessionMap)
  • faces23 - Spec1353IT (RequestCookie)
  • faces40 - Spec1582ApplicationMapIT
  • faces40 - Spec1582RequestCookieMapIT
  • faces40 - Spec1582ViewMapIT

@arjantijms arjantijms modified the milestones: 4.0.1, 4.0.1-TCK Nov 15, 2022
Copy link
Contributor

@brideck brideck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would appear that I was hasty with my initial review. Talked this over with some of our devs and the remaining faces40/cdi tests would need an update, too.

In the beans, lines like:

Map<String, String> headerMap = CDI.current().select(Map.class, HeaderMap.Literal.INSTANCE).get();

would need to be updated to:

Map<String, String> headerMap = CDI.current().select(new TypeLiteral<Map<String, String>>() {}, HeaderMap.Literal.INSTANCE).get();

in order to correctly test what's being attempted here.

@brideck
Copy link
Contributor

brideck commented Nov 16, 2022

It's also worth pointing out that the ApplicationMap test (both faces23 and faces40) are verifying behavior using the Mojarra-specific property jakarta.faces.BEANS_VALIDATION_AVAILABLE, which won't work for us. Can that be fixed here, too?

Alternatively, I could add it to the challenge for implementation-specific grievances (#1703). Or if it's preferred, I can open a new challenge for additional implementation-specific grievances.

@arjantijms
Copy link
Contributor Author

Map<String, String> headerMap = CDI.current().select(new TypeLiteral<Map<String, String>>() {}, HeaderMap.Literal.INSTANCE).get();

In a way for usability this is quite a step back, but a logical consequence of not allowing raw maps indeed. Maybe we can introduce something like a HeaderMap.TYPE to represent new TypeLiteral<Map<String, String>>() {} for Faces.Next.

Additionally set application map attribute explicitly.

Signed-off-by: Arjan Tijms <[email protected]>
@arjantijms arjantijms merged commit f0845e0 into jakartaee:master Nov 23, 2022
@arjantijms arjantijms deleted the 1710_parameterized_types branch November 23, 2022 20:42

return Boolean.toString(applicationMap.containsKey("jakarta.faces.BEANS_VALIDATION_AVAILABLE"));
return Boolean.toString(applicationMap.containsKey("barAttribute"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This same change needs to be made to the faces23/cdi version of this test, too.

@arjantijms arjantijms modified the milestones: 4.0.1-TCK, 4.0.2-TCK Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TCK Any issue having to do with the TCK not the API of SPEC text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TCK Challenge: CDI tests should inject Maps into parameterized types
4 participants