castor

Extend UnmarshalListener methods to receive the parent object also

Details

  • Type: New Feature New Feature
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: 1.3 rc1
  • Component/s: XML
  • Labels:
    None
  • Environment:
    all
  • Number of attachments :
    1

Description

Currently the methods of UnmarshalListener receive the 'target' object only. For JAXB the target object and its parent are required. And I do think that this is of interest for other Castor users working with listeners.


The problem: This issue will break the API!

In detail: UnmarshalListener is an interface which is communicated to users and extending its methods by an additional argument will break existing user code.

One other solution I though of is to create a new interface UnmarshalListenerInterface which is used instead of the current and create an abstract class UnmarshalListener that will take the role of an adapter between the new interface and old code... but still this would mean to change from:
public class MyListener implements UnmarshalListener to public class MyListener extends UnmarshalListener - if I ask the users for this change I can ask for the other also... and I would not work with such an ugly work around...

Or, I keep the old listener stuff and create a new second 'enhanced' listener interface and call both each time... having two listener setters, two listener getters, and loads of confusion what it is about... Which listener will be called first? Which listener will be maintained in future releases? and so on...

My personal favourite is to break the API! It hurts, but only once, and we're done with it.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Joachim Grüneis added a comment -

This patch now works with all implementations using the current UnmarshalListener interface! - But this interface is deprecated.

In addition:

  • a new interface is introduced which holds the new callback definitions
  • an adapter class is implemented which handles implementations of the deprecated interface
  • Unmarshaller and UnmarshalHandler have been changed to accept deprecated and new interface definition (using the adapter if a deprecated listener is set)
  • testcase mapping/listeners is renamed to mapping/old-listeners and tests listeners which are implemented using the deprecated interface
  • a new testcase mapping/listener is introduced working with the new listener interface

Two noteworthy things about this patch:

  1. new and deprecated interface have the same name but reside in different packages - this is
    1. perfect as just the import needs to be changed and then the new listener can be implemented instead of having a clumpsy name for the new interface
    2. madness as the two interfaces can for sure be confused

What is your opinion?

Show
Joachim Grüneis added a comment - This patch now works with all implementations using the current UnmarshalListener interface! - But this interface is deprecated. In addition: a new interface is introduced which holds the new callback definitions an adapter class is implemented which handles implementations of the deprecated interface Unmarshaller and UnmarshalHandler have been changed to accept deprecated and new interface definition (using the adapter if a deprecated listener is set) testcase mapping/listeners is renamed to mapping/old-listeners and tests listeners which are implemented using the deprecated interface a new testcase mapping/listener is introduced working with the new listener interface Two noteworthy things about this patch: new and deprecated interface have the same name but reside in different packages - this is perfect as just the import needs to be changed and then the new listener can be implemented instead of having a clumpsy name for the new interface madness as the two interfaces can for sure be confused What is your opinion?
Hide
Permalink
Werner Guttmann added a comment -

I have been talking to Steven to day, and he came up with the following idea:

a) Add the new methods to the interface.
b) Create a 'compliance base' class declares the old methods abstract and implements the new ones by simply 'voiding' them. This class would be for users that don't want to change their existing code. Well, they have to change the code, but it's a small change only.

What used to be 

public class MyListener implements UnmarshalListener { ...

now becomes 

public class MyListener extends BaseComplianceUnmarshalListener { ...
Show
Werner Guttmann added a comment - I have been talking to Steven to day, and he came up with the following idea: a) Add the new methods to the interface. b) Create a 'compliance base' class declares the old methods abstract and implements the new ones by simply 'voiding' them. This class would be for users that don't want to change their existing code. Well, they have to change the code, but it's a small change only. What used to be public class MyListener implements UnmarshalListener { ... now becomes public class MyListener extends BaseComplianceUnmarshalListener { ...
Hide
Permalink
Joachim Grüneis added a comment -

I do not like this approach.

As soon as I add the new methods to the old interface I have an interface that forces the users to implement old and new methods! This way the users will be confused about which method she will implement. Providing an abstract class (for the old style) that should be extended helps nothing! We would have to provide at least two abstract classes (old and new) instead of implementing the interface all users have to extend the classes . And, providing these abstract class to extend forces the user to modify exiting listener implementations - if I force our users to change implementation then I will force them to implement the new interface (without the old methods!)!

Having two interfaces - the old one being completely deprecated and the new one valid - is clearer. And handling the difference between the interfaces in a helper class which is completely hidden from the user is also clearer for the user.

Show
Joachim Grüneis added a comment - I do not like this approach. As soon as I add the new methods to the old interface I have an interface that forces the users to implement old and new methods! This way the users will be confused about which method she will implement. Providing an abstract class (for the old style) that should be extended helps nothing! We would have to provide at least two abstract classes (old and new) instead of implementing the interface all users have to extend the classes . And, providing these abstract class to extend forces the user to modify exiting listener implementations - if I force our users to change implementation then I will force them to implement the new interface (without the old methods!)! Having two interfaces - the old one being completely deprecated and the new one valid - is clearer. And handling the difference between the interfaces in a helper class which is completely hidden from the user is also clearer for the user.
Hide
Permalink
Ralf Joachim added a comment -

I would also prefer Joachims approach having a new interface without old methods and hidden adapters.

Show
Ralf Joachim added a comment - I would also prefer Joachims approach having a new interface without old methods and hidden adapters.
Hide
Permalink
Werner Guttmann added a comment -

Fine by me. But please do make sure that the documentation is complete with regards to this change. And if there is no documentation at all, how about adding it .. ?

Show
Werner Guttmann added a comment - Fine by me. But please do make sure that the documentation is complete with regards to this change. And if there is no documentation at all, how about adding it .. ?
Hide
Permalink
Joachim Grüneis added a comment -

Solved with the patch as uploaded before...

  • introduced a new interface
  • set the existing interface to deprecated
  • introduced an adapter from deprecated interface to new (which isn't actively used by users)
Show
Joachim Grüneis added a comment - Solved with the patch as uploaded before... introduced a new interface set the existing interface to deprecated introduced an adapter from deprecated interface to new (which isn't actively used by users)

People

  • Assignee:
    Joachim Grüneis
    Reporter:
    Joachim Grüneis
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: