google-code-export / protostuff

Automatically exported from code.google.com/p/protostuff
Apache License 2.0
1 stars 1 forks source link

Multiple runtime schema contexts (IdStrategy groups) #140

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Hi, I think it would be good to have more control over a process of creating 
runtime schema, especially a possibility to select fields to include. I know I 
can pass "exclusions" argument to createFrom() method, but I think it's totally 
wrong. RuntimeSchema works on-demand, you don't preconfigure your schemas, but 
use @Tag, @Deprecated or transient and then call getSchema() whenever you want. 
But if you want to exclude some fields you have to register all your schemas 
before using protostuff or don't use getSchema() at all.

Next thing are multiple schemas for one pojo, preferably divided into the whole 
contexts, so if you serialize object A using some context and it contains 
object B then B will be automatically serialized with the same context. It's 
useful e.g. if you want to send the same objects to db and to the web client, 
but some fields should be omitted in one place or another. You definitely don't 
want to send User.passwordHash field to the client :-)

It is possible to have multiple contexts by initializing multiple IdStrategy 
objects, but if you have 20 pojos and you want to customize their schemas, it's 
a lot of work and it's very error-prone.

I have created quite simple annotation-based solution to this. You annotate 
fields with @SchemaGroup and pass a list of group names. Then you instantiate 
DefaultIdStrategy with other list of groups. If idStrategy and a field has at 
least one common group then this field is included into schema. If a field 
doesn't have @SchemaGroup annotation or you don't instantiate DefaultIdStrategy 
with a list of groups then field is included as well. My changes should not 
have any impact on existing code. I didn't run automating tests because they 
didn't pass even without any changes.

Example usage is here: https://gist.github.com/brutall/5f6c9c45ce4204b5da35 . 
It will output:

{"inner":{"propAll":true,"prop1":true,"prop2":true,"propBoth":true}}
{"inner":{"propAll":true,"prop1":true,"propBoth":true}}
{"inner":{"propAll":true,"prop2":true,"propBoth":true}}
{"inner":{"propAll":true}}

I include the patch for 1.0.7 version, but I think it should work for trunk. 
You may have to fix formatting.

Original issue reported on code.google.com by Brut.alll on 10 Feb 2013 at 10:30

Attachments:

GoogleCodeExporter commented 9 years ago
Your approach has the initial good parts.  The problem is that there will be 
extra overhead because of the re-creation of fields.

I've committed a solution for this that allows you to include fields for a 
group, as well a exclude.

The side-by-side diff is here 
(http://code.google.com/p/protostuff/source/diff?spec=svn1591&r=1591&format=side
&path=/trunk/protostuff-runtime/src/main/java/com/dyuproject/protostuff/runtime/
RuntimeSchema.java)

Take a look at the testcase to see the usage: 
http://code.google.com/p/protostuff/source/browse/trunk/protostuff-runtime/src/t
est/java/com/dyuproject/protostuff/runtime/FilterFieldsByGroupTest.java

This approach is the most flexible and the least overhead.
Let me know if you encounter problems.

Original comment by david.yu...@gmail.com on 11 Feb 2013 at 5:43

GoogleCodeExporter commented 9 years ago
I didn't care about caching, because these are just fractions of a second and 
only at the initialization time - this isn't a problem even on Android. I don't 
think there's much sense in having >5 contexts, usually there will be 2-3 and 
that means only 1-2 redundant analysis.

But of course your solution is very nice, especially possibility to choose 
between including or excluding from groups. I will use it in my project and 
report back if I run into any problems.

Thanks!

Original comment by Brut.alll on 11 Feb 2013 at 6:21

GoogleCodeExporter commented 9 years ago
Ahh and I found that even if Protostuff is built on interfaces, etc., many 
things are tightly coupled. Initially I wanted to add schema groups by writing 
a module to protostuff, not by modifying its sources. I wanted to write 
alternative createFrom() method, but original createFrom() is called statically 
from Lazy class, so I couldn't replace it. Lazy is very simple class, so I 
could write my own alternative, but it is hardcoded in DefaultIdStrategy. I 
tried to override DefaultIdStrategy behavior by delegation, but 
getSchemaWrapper() method is called in several places internally. So I failed.

Also I wanted to add joda time support, I dive into all these field factories, 
etc., but found it's also not possible from the outside. I have succeeded by 
hacking private __inlineValues field and now I don't understand why there is no 
clean way to do that. I think possibility to register a custom inline factories 
would be very powerful.

Don't take my words as criticizing you, I understand your priority is 
performance, not flexibility. I'm just curious why Protostuff is so closed to 
the outside, even if it doesn't have to.

Original comment by Brut.alll on 11 Feb 2013 at 7:09

GoogleCodeExporter commented 9 years ago
You definitely can add custom inline fields from the outside.  That is what 
http://code.google.com/p/protostuff/wiki/Delegate is for.
They have the same semantics.  Internally they are treated the same.

Not sure what you meant by closed to the outside?
Basically, I prefer not to have external dependencies on the core modules.
A module for 3rd party libraries is preferable.

Can you post your joda-time inline hack?  There's definitely a clean way of 
doing that (which we can put in a new module).

Original comment by david.yu...@gmail.com on 12 Feb 2013 at 9:44

GoogleCodeExporter commented 9 years ago
Also, the static RuntimeSchema.createFrom can be moved to IdStrategy, but 
without the static modifier (to allow the impl to override).
Does that solve your problem?

Original comment by david.yu...@gmail.com on 12 Feb 2013 at 9:49

GoogleCodeExporter commented 9 years ago
IdStrategy.newSchema added.

Original comment by david.yu...@gmail.com on 12 Feb 2013 at 10:48

GoogleCodeExporter commented 9 years ago
Ahh, I missed delegates feature :-/ I have analyzed RuntimeFieldFactory to know 
how protostuff chooses field (de)serializing method, but I missed first check 
in getFieldFactory() method. Seems it's exactly what I need. My hack was quite 
simple: I have implemented RuntimeFieldFactory<DateTime> and added it to 
__inlineValues using reflection API.

I apologize for complaining - it seems a problem wasn't with protostuff, but 
with me ;-)

And about static methods in RuntimeSchema: I think it's usually better to not 
write too much logic statically. If there is a need to have global, quick 
access to some functionality then it's better to write non-static logic and 
simple static helpers using singleton object. This way you can instantiate 
multiple contexts, override or reconfigure some of them, but still if you just 
need one instance of a tool then you can use convenient global helpers. You did 
exactly what I said: IdStrategy is context, RuntimeEnv.ID_STRATEGY is singleton 
object and RuntimeSchema is static helper, but unfortunately the last one 
doesn't forward all functionality to IdStrategy, but implement part of the 
logic by itself. Still you can use RuntimeSchema with your own context or with 
the default one, but as we can see it's not that easy to change its behaviour.

Yes, I think it would be better if createFrom() methods would be moved to 
IdStrategy interface and RuntimeSchema would just forward to it. I'm not sure 
if that would help much in my above case, because DefaultIdStrategy is final, 
so it's not possible to override its methods just like that, but still I think 
it's better.

Original comment by Brut.alll on 12 Feb 2013 at 10:50

GoogleCodeExporter commented 9 years ago
I'll mark this issue closed.  Let me know if more changes are needed.
Thanks for the initial patch!

Original comment by david.yu...@gmail.com on 20 Feb 2013 at 1:14