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

Factor out custom property editors and PropertyEditorRegistrar behaviour from CustomEditorConfigurer [SPR-4361] #9039

Closed
spring-projects-issues opened this issue Jan 21, 2008 · 6 comments
Assignees
Labels
in: core status: declined type: enhancement

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Jan 21, 2008

Dave Syer opened SPR-4361 and commented

Factor out custom property editors and PropertyEditorRegsitrar behaviour from CustomEditorConfigurer, so that the map of custom editors can be used to register with a variety of PropertyEditorRegistry types, not just the BeanFactory. There is an implementation already in Spring Batch (https://springframework.svn.sourceforge.net/svnroot/springframework/spring-batch/trunk/spring-batch-infrastructure/src/main/java/org/springframework/batch/support/DefaultPropertyEditorRegistrar.java) which would make more sense in Core.

P.S. please backport to 2.0.9 as well if it's not too difficult.


Affects: 2.5.1

1 votes, 2 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 5, 2008

Juergen Hoeller commented

Dave, we've actually deprecated the "customEditors" property in CustomEditorConfigurer in 2.5, mainly to indicate that those editors cause a need for locking since the specified editor instances aren't thread-safe.

So I wonder about such a PropertyEditorRegistrar implementation: If you defined this as a singleton bean, you'd run into the same problem. Any code accepting that registrar would have to be aware of the need for locking... why by default hardly any such code will be (in contrast to the BeanFactory which is aware of that need when using CustomEditorConfigurer).

In other words: Is it even recommendable to define a PropertyEditorRegistrar with such declarative editor instances? I would argue: No, it isn't... PropertyEditorRegistrar should rather be written programmatically, creating the editor instances for each call and having full control over their constructor arguments etc. Arguably programmatic configuration is nicer than XML configuration here anyway...

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 8, 2008

Dave Syer commented

This is basically because PropertyEditor sucks, right? What we really need is another interface that does the same job - why don't we just bin the property editor completely instead of deprecating the only mechanism there is to configure them? Programmatically configuring converters is not a great idea if they have properties that need to be set by the user (e.g. date format). If I need a way to plug converter strategies into a custom object, what should I do? There should be a "Spring way" to do this, that can be adopted across the portfolio. If it has to be PropertyEditor then I think you should provide the synchronization features in a base class as well, so that other services can take advantage of the functionality that is currently hidden in the BeanFactory implementations.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 8, 2008

Juergen Hoeller commented

Well, that common interface is PropertyEditorRegistrar at present - which should be fine as a strategy interface for registering converter strategies on any given PropertyEditorRegistry. This is the resaon why we deprecated the registration of direct PropertyEditor instances: We've got the PropertyEditorRegistrar mechanism instead - since Spring 1.2.6 already! So it's by no means the "only" mechanism deprecated there; it's the less preferable of two mechanisms being deprecated now.

So for plugging converter strategies into a custom object, let that object accept a PropertyEditorRegistrar. This doesn't require any synchronization, neither explicit nor implicit. The registrar needs to be implemented programmatically, but frankly, this is no big deal and even quite appropriate in many cases. Take your DateFormat example: DateFormats are themselves not thread-safe, so you better recreate them within a registrar and pass them into a non-reused PropertyEditor instance. Matches quite nicely. The same applies to NumberFormats.

From that perspective, it would be counterproductive to make the BeanFactory impl's editor synchronization available in standalone form. The PropertyEditorRegistrar approach is a better choice, in particular given that DateFormats/NumberFormats/MessageFormats aren't thread-safe either, and also given that BeanWrapper instances are recreated for each target object too. BeanWrapper doesn't need synchronization at present, so suddenly making it synchronize on shared PropertyEditor instances feels wrong from that perspective as well.

Granted, we will revisit the topic for Spring 3.0 in the course of its unified expression language support and revised binding facilities. For the Spring 2.5 series, the above PropertyEditorRegistrar story won't change: This has to good enough for the time being.

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 10, 2008

Dave Syer commented

I still don't fully understand. The Javadocs for PropertyEditorRegistrar.registerCustomEditors say

"It is expected that implementations will create brand new PropertyEditors instances for each invocation of this method (since PropertyEditors are not threadsafe)."

This does make sense, but it also seems like Spring is copping out - there might be PropertyEditors in the supplied registry that I really can't, and don't want to, re-create for every method call, especially ones that have been set up with some configuration, dependency injection etc. Can't we do better than this (even if it is 3.0)?

Again, doesn't it basically come down to the fact that PropertyEditor is the wrong interface for a conversion strategy?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Apr 2, 2008

Stephen R. Saucier commented

I don't disagree that programmatic configuration of the PropertyEditorRegistrar isn't that onerous or could in some cases be desirable, but I don't think that should preclude a declarative configuration option -- wondering why I couldn't do such a thing with Spring I wrote a "ConfigurablePropertyEditorRegistrar that implements BeanFactoryAware and then retrieves an instance of the PropertyEditor from the BeanFactory. Of course, in order to meet the general contract of the PropertyEditorRegistrar, you have to make sure that any PropertyEditors retrieved using the BeanFactory are configured to have prototype scope. What would be nice is to have a mechanism to override the scope of a bean when using the BeanFactory, something like beanFactory.getBean("propertyEditor", Scope.PROTOTYPE); in order to ensure that you always get a new instance of the object, regardless of how it has been configured. Admittedly though, I haven't thought this plan through entirely and doing so would probably undermine the ability of the IoC container to manage object instances correctly, but no more so than constructing an instance of the object myself, not through the BeanFactory...

Anyway, back to the matter at hand, my "ConfigurablePropertyEditorRegistrar" class basically has a setCustomEditors(Map customEditors) method like the CustomEditorConfigurer class but instead takes a map of class names / bean name pairs, using the BeanFactory to retrieve an instance of the PropertyEditor object and then calling registry.registerCustomEditor(...) method. It seems like something along these lines could be implemented in Spring, probably with the additional ability to scope a particular PropertyEditor to a specific property path (that is, using the registerCustomEditor method that takes three arguments).

Maybe it is just me, but after using Spring, any time I have to instantiate an object, I think "why can't spring do this for me?" so having to create the PropertyEditor instances myself in a PropertyEditorRegistrar just seems contrary to the goals of Spring...

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 18, 2012

Rossen Stoyanchev commented

This issue has been resolved through a selective bulk update, as part of a larger effort to better manage unresolved issues. To qualify for the update, the issue was either created before Spring 3.0 or affects a version older than Spring 3.0 and is not a bug.

There is a good chance the request was made obsolete, or at least partly outdated, by changes in later versions of Spring including deprecations. It is also possible it didn't get enough traction or we didn't have enough time to address it. One way or another, we didn't get to it.

If you believe the issue, or some aspects of it, are still relevant and worth pursuing at present you may re-open this issue or create a new one with a more up-to-date description.

We thank you for your contributions and encourage you to become familiar with the current process of managing Spring Framework JIRA issues that has been in use for over a year.

@spring-projects-issues spring-projects-issues added status: declined type: enhancement in: core labels Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core status: declined type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants