castor

SourceGenerator: PropertyChangeListeners for bound properties should not be serialized

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 0.9.9
  • Fix Version/s: 0.9.9.1
  • Component/s: XML code generator
  • Labels:
    None
  • Number of attachments :
    6

Description

SourceGenerator generates propertyChangeListeners Vector for JavaBeans generated from XSD. This member should be transient so that change listeners are not serialized/deserialized.

Patch (against CVS HEAD) to SourceFactory.java included.

Attachments

  2. Text File
    Castor1152-configurable.patch
    04/Aug/05 11:26 AM
    6 kB
    David Green
  3. Text File
    ctfrun_out.txt
    04/Aug/05 10:18 AM
    218 kB
    David Green
  4. Text File
    ctfrun_out2.txt
    04/Aug/05 10:18 AM
    179 kB
    David Green
  5. Text File
    SourceFactory_PropertyChangeSupport.java.patch
    22/Aug/05 12:14 PM
    5 kB
    David Green
  6. Text File
    SourceFactory.java.patch
    08/Jun/05 4:24 PM
    2 kB
    David Green

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
David Green added a comment -

A CTF test case.
It's not clear how to make JUnit check for a transient keyword in the test case.

Show
David Green added a comment - A CTF test case. It's not clear how to make JUnit check for a transient keyword in the test case.
Hide
Permalink
David Green added a comment -

How can I find out if there is any activity or interest in this issue? What else can I do to get the patch applied?

Show
David Green added a comment - How can I find out if there is any activity or interest in this issue? What else can I do to get the patch applied?
Hide
Permalink
Werner Guttmann added a comment -

David, I know it can be frustrating to not get any feedback upfront. If I were you, I'd send an email to the dev mailing list asking for feedback on the validity of the approach, and inquire whether somebody can take ownership of this issue. If, in the meantime, you could try to run the CTF suite and see whether everything still runs fine, you'd definitely decreease somebody else's workload.

Show
Werner Guttmann added a comment - David, I know it can be frustrating to not get any feedback upfront. If I were you, I'd send an email to the dev mailing list asking for feedback on the validity of the approach, and inquire whether somebody can take ownership of this issue. If, in the meantime, you could try to run the CTF suite and see whether everything still runs fine, you'd definitely decreease somebody else's workload.
Hide
Permalink
David Green added a comment -

CTF test suite run output.

Show
David Green added a comment - CTF test suite run output.
Hide
Permalink
David Green added a comment -

CTF test suite run output file (CTF run without patch)

Show
David Green added a comment - CTF test suite run output file (CTF run without patch)
Hide
Permalink
David Green added a comment -

I've run the CTF suite against CVS HEAD with and without the patch, and attached the output of the CTF suite. (ctfrun_out.txt is output without patch, ctfrun_out2.txt is output with patch). The patch doesn't break any CTF tests.

Show
David Green added a comment - I've run the CTF suite against CVS HEAD with and without the patch, and attached the output of the CTF suite. (ctfrun_out.txt is output without patch, ctfrun_out2.txt is output with patch). The patch doesn't break any CTF tests.
Hide
Permalink
Werner Guttmann added a comment -

Just following up with regards to your email to the dev mailing list. I really don't believe that the listeners should be serialized at all, but in order not to break that behaviour, making this configurable per castor.properties sounds about right to me.

Show
Werner Guttmann added a comment - Just following up with regards to your email to the dev mailing list. I really don't believe that the listeners should be serialized at all, but in order not to break that behaviour, making this configurable per castor.properties sounds about right to me.
Hide
Permalink
David Green added a comment -

Attached a patch that permits configuration of this feature (on/off) via the standard properties file with the default setting being equivalent to pre-patch behaviour.

Show
David Green added a comment - Attached a patch that permits configuration of this feature (on/off) via the standard properties file with the default setting being equivalent to pre-patch behaviour.
Hide
Permalink
Gregory Block added a comment -

Totally disagree.

1) PropertyChangeListener interface is not serializable.
2) The beans API knows this, and expects you to use PropertyChangeSupport to implement the listeners; you then serialize the PropertyChangeSupport object.
3) You then hook up your API to the PropertyChangeSupport API via delegation, which then correctly handles proxies and does the work of allowing serialization to happen on the list of listeners, only some of which might be serializable.

Either move to PropertyChangeSupport in all generated objects, or make darn sure we never serialize by marking it transient.

Show
Gregory Block added a comment - Totally disagree. 1) PropertyChangeListener interface is not serializable. 2) The beans API knows this, and expects you to use PropertyChangeSupport to implement the listeners; you then serialize the PropertyChangeSupport object. 3) You then hook up your API to the PropertyChangeSupport API via delegation, which then correctly handles proxies and does the work of allowing serialization to happen on the list of listeners, only some of which might be serializable. Either move to PropertyChangeSupport in all generated objects, or make darn sure we never serialize by marking it transient.
Hide
Permalink
Gregory Block added a comment -

And by all means, break current and historical behavior.

Show
Gregory Block added a comment - And by all means, break current and historical behavior.
Hide
Permalink
David Green added a comment -

Good point: using PropertyChangeSupport would permit runtime choice of listeners participating in serialization.

Show
David Green added a comment - Good point: using PropertyChangeSupport would permit runtime choice of listeners participating in serialization.
Hide
Permalink
Werner Guttmann added a comment -

Alright, not knowing a lot about PropertyChangeListeners et alias, what does this translate to ? It looks like the configuration mechanism via castor.properties is not required.

Show
Werner Guttmann added a comment - Alright, not knowing a lot about PropertyChangeListeners et alias, what does this translate to ? It looks like the configuration mechanism via castor.properties is not required.
Hide
Permalink
Gregory Block added a comment -

Correct - no configuration mechanism, as it shouldn't exist.

Basically, the purpose of using PropertyChangeSupport is twofold: Add support for proxied listeners, and properly support serialization.

If you don't use the PropertyChangeSupport methods, you have to code it all yourself; moreover, because you've got a raw list, you then also have to code your own manual serialization/deserialiation methods - something which this just can't do.

PropertyChangeSupport implements all the required methods for serializing and deserializing safely; it also implements all the helper methods needed, in a way that supports proxyied listeners..

Please move to a PCS-based implementation to eliminate this and other problems, as that's the intended implementation of PropertyChangeListener support in beans objects.

As for what this is for? I have no idea. I never use sourcegen.

Show
Gregory Block added a comment - Correct - no configuration mechanism, as it shouldn't exist. Basically, the purpose of using PropertyChangeSupport is twofold: Add support for proxied listeners, and properly support serialization. If you don't use the PropertyChangeSupport methods, you have to code it all yourself; moreover, because you've got a raw list, you then also have to code your own manual serialization/deserialiation methods - something which this just can't do. PropertyChangeSupport implements all the required methods for serializing and deserializing safely; it also implements all the helper methods needed, in a way that supports proxyied listeners.. Please move to a PCS-based implementation to eliminate this and other problems, as that's the intended implementation of PropertyChangeListener support in beans objects. As for what this is for? I have no idea. I never use sourcegen.
Hide
Permalink
David Green added a comment -

PropertyChangeSupport does indeed appear to be the least intrusive approach, and does follow the serialization behaviour as intended by the Java standard. This patch causes the source generator to generate JavaBeans with java.beans.PropertyChangeSupport instead of Vector.

Show
David Green added a comment - PropertyChangeSupport does indeed appear to be the least intrusive approach, and does follow the serialization behaviour as intended by the Java standard. This patch causes the source generator to generate JavaBeans with java.beans.PropertyChangeSupport instead of Vector.
Hide
Permalink
Andrew Fawcett added a comment -

Small change to submitted patch to retain existing signature of the removePropertyChangeListner method.

Show
Andrew Fawcett added a comment - Small change to submitted patch to retain existing signature of the removePropertyChangeListner method.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.