AMQP-202

[SamNelson]

I've changed the DefaultClassMapper and the JsonMessageConverter to use JavaTypeFactory. The DefaultClassMapper has been renamed to the DefaultJavaTypeMapper. The getter and setter have been renamed to use the DefaultJavaTypeMapper. This is a small API change. I hope that is OK.

[SamNelson]

I've reinstated the ClassMapper and DefaultClassMapper. DefaultClassMapper now implements both ClassMapper and JavaTypeMapper interfaces.

[dsyer]

I don't see your names on the committer list yet. Can you fill in this form, please (as per the README): https://support.springsource.com/spring_committer_signup?

[SamNelson]

I've filled out the form. My confirmation number is 16820111116091809.

-----Original Message-----
From: Dave Syer [mailto:[email protected]]
Sent: Wednesday, November 16, 2011 10:36 AM
To: Sam Nelson
Subject: Re: [spring-amqp] AMQP-202 (#14)

I don't see your names on the committer list yet. Can you fill in this form, please (as per the README): https://support.springsource.com/spring_committer_signup?

---

Reply to this email directly or view it on GitHub:
#14 (comment)

[dsyer]

Thanks. Now I want to understand the code a bit. How come Default* has to implement JavaTypeMapper? I thought that was purely a JSON concern, so should only be needed in Json*Mapper?

[SamNelson]

Probably no good reason. I'll set Default* back to the way it was, and change the Json*Mapper just to use the JavaTypeMapper

[SamNelson]

This change relies on Jackson's ObjectMapper.readValue(String, JavaType) call. It made sense to me to replace the ClassMapper with a JavaTypeMapper since the ClassMapper was not used outside of the JsonMessageConverter in spring-amqp. What I was not sure of was whether it was better to leave the unused interface for someone who was using it for different purposes or signal the required API change in JsonMapperMessageConverter with a compilation error. I didn't really see a clean way to avoid the API change to JsonMapperMessageConverter, but I can probably work something out.

[dsyer]

OK, sorry, I'm catching up here now. ClassMapper is only used by JsonMessageConverter, and since that is already tied to Jackson, we could have saved some hassle by using JavaType from the beginning (or a Spring abstraction for the same thing). Given that, I think it might be preferable to use instanceof checks in JsonMessageConverter (e.g. if (classMapper instanceof JavaTypeMapper)) instead of having two fields for mappers. It's a hack, but it's our fault for getting it wrong in the first place, and we can comment the instanceof to document why it is necessary. That way we only need one DefaultClassMapper, which happens to implement the new interface. the result should be closer to your original design, but without the backwards incompatibility problem.

One last thing, and we can close this down: can you add an extra test to JsonMessageConverterTest that would fail using a ClassMapper but succeed using a JavaTypeMapper? E.g. a test that correctly serializes and deserializes a collection of SimpleTrade?

[SamNelson]

Sounds good. I think I've figured out how to keep the JavaTypeFactory and JavaType within the DefaultClassMapper. This will allow me to remove the JavaTypeMapper interface, and use only the ClassMapper.

There's also a problem with the current implementation which I need to fix and test.

[olegz]

Sam

Make sure next time you make changes on the branch instead of the master. Just go through this https://github.com/SpringSource/spring-integration/wiki/Contributor-Guidelines and it will become clear
Cheers and thank you for contributing