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

Add UUIDConverter #1819

Closed
hantsy opened this issue Jun 12, 2023 · 14 comments
Closed

Add UUIDConverter #1819

hantsy opened this issue Jun 12, 2023 · 14 comments
Labels
mojarra-implemented API issue implemented by Mojarra myfaces-implemented API issue implemented by MyFaces
Milestone

Comments

@hantsy
Copy link

hantsy commented Jun 12, 2023

Faces should add a default converter for UUID type...till now I have to create an custom for myself.

JPA has already add UUID as basic type and also support it as ID.

Originally posted by @hantsy in #752 (comment)

@hantsy
Copy link
Author

hantsy commented Jun 12, 2023

I remember in the next Jakarta EE plan dicussion doc(Google docs), there is a common placeholder for the future Jakarta.

I hope there is a common ConversionService to register a collection of common generic Converter<S,T> and serve all converters in the Jakarta specs, such as JPA, Faces, Rest, Config, etc.

BalusC added a commit to eclipse-ee4j/mojarra that referenced this issue Jun 18, 2023
BalusC added a commit to eclipse-ee4j/mojarra that referenced this issue Jun 18, 2023
BalusC added a commit to eclipse-ee4j/mojarra that referenced this issue Jun 18, 2023
Add message translations for de/en/es/pt
@BalusC
Copy link
Member

BalusC commented Jun 18, 2023

I added UUIDConverter for 4.1, cc: @arjantijms

Next step: translations of faces messages for following languages which I'm not familiar with:

  • fr
  • ja
  • ko
  • zh_CN
  • zh_HK
  • zh_TW

Any volunteers? :)

@hantsy
Copy link
Author

hantsy commented Jun 19, 2023

Chinese translation eclipse-ee4j/mojarra#5244

BalusC added a commit to eclipse-ee4j/mojarra that referenced this issue Jun 19, 2023
Added French translation, unverified, it was a best guess based on
existing NumberConverter.PERCENT translation.
@BalusC
Copy link
Member

BalusC commented Jun 19, 2023

Thank you very much! I've merged your Chinese translation into the PR. Then I added a best guess of the French translation.

Now only pending:

  • ja
  • ko

Translations are not mandatory for the spec btw. It'll in any case fall back to English as default.

@arjantijms
Copy link
Contributor

I hope there is a common ConversionService to register a collection of common generic Converter<S,T> and serve all converters in the Jakarta specs, such as JPA, Faces, Rest, Config, etc.

I brought this forward a long time ago:

https://arjan-tijms.omnifaces.org/2014/08/high-time-to-standardize-java-ee.html

@pizzi80
Copy link

pizzi80 commented Jun 21, 2023

@BalusC you could ask ChatGPT to translate, if you ask properly it will respect the properties template ;)

https://chat.openai.com/

@pizzi80
Copy link

pizzi80 commented Jun 21, 2023

I'm working on "basic" Converters optimization (I'll create a PR within an hour)
and I studied a bit the algorithm to create a Converter.

I made some tests and I suspect that there are some minor "bugs",
or something that should be clarified ...

For example:
to create an @ApplicationScoped stateless Converter over
the basic IntegerConverter at the moment
I use the following workaround:

@ApplicationScoped
@FacesConverter(forClass = Integer.class)
public class IntegerConverter extends jakarta.faces.convert.IntegerConverter {}

But in this way it does not override the default Faces Converter at runtime.

To make it work I also have to declare it inside faces-config.xml with

<converter>
  <converter-for-class>java.lang.Integer</converter-for-class>
  <converter-class>com.my.package.IntegerConverter</converter-class>
</converter>

In this way it works, in fact if I inspect the class hashcode, the instance is always the same

but on Faces side,
if i put a log inside the class InstanceFactory I see that the algorithm instantiate
a new IntegerConverter during every input processing anyway ....

I don't know if this behavior or other minor bugs are causing or contributing
to the following bugs or not... it probably needs more testing

eclipse-ee4j/mojarra#5110
eclipse-ee4j/mojarra#5119

Overall the algorithm is very complex and it is implemented
in a very complex way! :/

Probably it's time to refactor all the process as @arjantijms said almost 10 years ago

@hantsy
Copy link
Author

hantsy commented Jun 22, 2023

If the faces converter is managed by CDI, it should be found by CDI bean manager directly, no need register it in faces-config.xml.

@hantsy
Copy link
Author

hantsy commented Jun 22, 2023

And @FacesConverter is to replace the faces-config.xml config?

@pizzi80
Copy link

pizzi80 commented Jun 23, 2023

@hantsy Yes it is exactly what I thought, but if I remove the declaration inside faces-config.xml it doesn't work as expected... (or at least how I expect it should works)

Evenmore, by putting some log inside mojarra's internal algo,
I discovered that the Converter get instanciated anyway but not "returned"
... basically a waste of resources ...

I need more time to test this, but I don't know if it's time to because at the end of the day
the Faces API can't be changed before the Faces 5 version...

@arjantijms
Copy link
Contributor

arjantijms commented Jun 23, 2023

but if I remove the declaration inside faces-config.xml it doesn't work as expected...

For the next version we should probably deprecate declaring inside faces-config.xml and thereafter prune that. The difference in expectations is (I think) on ongoing source of issues and confusion.

@hantsy
Copy link
Author

hantsy commented Jun 24, 2023

@arjantijms @pizzi80 I have added a UUIDConverter in my jakartaee10-starter-boilerplate, used it directly in the facelets without adding declaration in faces-config.xml, it works on Glassfish 7.0.5.

@pizzi80
Copy link

pizzi80 commented Jun 24, 2023

UUIDConverter it's not a default basic converter and you are explicitly declaring it in the view

Try to override the IntegerConverter without declaring it in the view using the forClass attribute in the java annotation

BalusC added a commit to eclipse-ee4j/mojarra that referenced this issue Jul 22, 2023
@BalusC BalusC added this to the 4.1 milestone Jul 22, 2023
@BalusC BalusC added the mojarra-implemented API issue implemented by Mojarra label Jul 22, 2023
@tandraschko tandraschko added the myfaces-implemented API issue implemented by MyFaces label Jul 25, 2023
@tandraschko
Copy link

any more tasks? implemented in both impls, we could close this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mojarra-implemented API issue implemented by Mojarra myfaces-implemented API issue implemented by MyFaces
Projects
None yet
Development

No branches or pull requests

5 participants