cognitect / transit-java

transit-format implementation for Java
Apache License 2.0
62 stars 21 forks source link

write handler for java.lang.Object is ignored #23

Closed mikub closed 7 years ago

mikub commented 8 years ago

current version does not allow for adding a generic handler for java.lang.Object. This however is desirable and would be tremendously useful when i just want to add a generic handler for objects that don't have a specific custom handler to prevent runtime exceptions ("java.lang.Exception: Not supported") whenever some new object sneaks onto the transit data. Though an application should always sanitize the data before sending them over the wire (to make sure only 'serializable' data are sent), the application might not be aware of what transit handlers are currently supported. Sanitizing the data before handing them over to transit would thus lead to handler duplication and tight coupling. By adding a generic handler for java.lang.Object a user can thus decide - e.g. for the remaining objects that don't have handler I want them serialized via nippy or converted to String etc.

Here is my pull request - [https://github.com/cognitect/transit-java/pull/22]

mikub commented 8 years ago

when creating the pull request I realized that this likely is a simple bug (I thought originally this was somehow intentional). I almost made the same mistake when tweaking the for loop (see my first and second commit to the pull request) - the thing with java's for loops is that the for condition is evaluated at the beginning of the loop iteration while the mutation of its value is performed at the end of each iteration. The last iteration (for the java.lang.Object) simply did not happen.

dchelimsky commented 8 years ago

Thanks for reporting this. I closed the PR as we don't accept PR's (see https://github.com/cognitect/transit-java#contributing), but leaving the issue open for discussion.

Need to give this some thought before responding, but I'll follow up soon.

whilo commented 7 years ago

I just would like to point out that I am struggeling with overriding the java.util.Map handler atm. too. But this is rather a hacky way of patching transit, fressian and potential other serialization libraries to not throw away record type information in https://github.com/replikativ/incognito/, so that they can be deserialized later again. If there is a better way, then I would definitely appreciate getting rid of these "hacks". A particular problem is that types can dispatch an arbitrary protocol in Clojure if there are multiple matching base types. If this is supposed to be the way (and the type information in records is seen as disposable), then at least dealing with IRecord alone would help, instead of subtyping and overwriting the java.util.Map handler. The latter doesn't seem to work anymore with the new transit versions.

mikub commented 7 years ago

Any update on this? It is a simple bug / programmer's mistake (wrong condition in a for loop) that takes few seconds to fix...

Imho what you currently have there is equivalent of the following. Though probably not originally intended, the loop (or its final iteration) will never get executed. Only in our case it is the object inheritance hierarchy and not increment: int a = 0; for(int i = ++a ; i >=1 ; i = i++) { //do something }

What I am asking you here is to simply change i >=1 to i >1.

dchelimsky commented 7 years ago

@mikub the change you're proposing doesn't work because getHandler searches for handlers for ancestor Classes before implemented Interfaces, so any direct subs of Object that implement Interfaces that are keys in the handler map would fall through to that handler. Searching for Interfaces first would be a breaking change for systems with custom handlers registered for both classes and interfaces.

If I understand the problem you're trying to solve, I think support for registering a default handler which gets used when no handlers are found for the class of the object, any of its ancestor classes (other than Object), or implemented interfaces would solve it. WDYT?

mikub commented 7 years ago

Hi David, thanks for your quick response!

searches for handlers for ancestor Classes before implemented Interfaces

Oh that's a good point, I ran locally a test case but didn't test for any interface(s) ;)

support for registering a default handler

That is exactly what I am after! (thinking out loud: ) in the light of your comments I guess it would have to be implemented as an additional if after the class/interface inheritance checks.

Cheers Miro

dchelimsky commented 7 years ago

Yep, that's the idea, plus support for default handler registration. I likely won't get to this before next Friday at the earliest, but it's now on the radar with specific direction so it'll happen sooner or later.

dchelimsky commented 7 years ago

There was actually a request for this in #19, so I'm going to close this now and follow up there.

dchelimsky commented 7 years ago

@mikub I posted PR #27 just for review. Please feel free to comment or try out that branch.