jakartaee / faces

Jakarta Faces
Other
109 stars 55 forks source link

Add UUIDConverter #1819

Closed hantsy closed 1 year ago

hantsy commented 1 year ago

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 https://github.com/jakartaee/faces/issues/752#issuecomment-1587509013

hantsy commented 1 year ago

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 commented 1 year ago

I added UUIDConverter for 4.1, cc: @arjantijms

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

Any volunteers? :)

hantsy commented 1 year ago

Chinese translation https://github.com/eclipse-ee4j/mojarra/pull/5244

BalusC commented 1 year ago

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:

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

arjantijms commented 1 year ago

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 commented 1 year ago

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

https://chat.openai.com/

pizzi80 commented 1 year ago

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

https://github.com/eclipse-ee4j/mojarra/issues/5110 https://github.com/eclipse-ee4j/mojarra/issues/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 commented 1 year ago

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 commented 1 year ago

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

pizzi80 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

tandraschko commented 1 year ago

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