tentative development for #38 #39

bytesandwich · 2012-07-27T06:41:45Z

bytesandwich · 2012-07-27T06:42:56Z

Sorry about the whitespace change diff. I tried to rebase it out, but I wasn't able to smooth over my local/github repos today.

bytesandwich · 2012-07-27T16:05:19Z

I'm not sure how to go from annotated class to schema. I think having the schema objects with its own type hierarchy makes sense, which can be manipulated to add schema values that aren't inferred automatically. I think that the schema should generation at least needs to keep the basicbeandescription in context, to be query-able for custom generation.

bytesandwich · 2012-07-27T16:40:01Z

Currently, the schema aware serializers can just generate the schema themselves. They could instead return a proper schema subclass rather than a jsonnode, and schema could be a subclass of jsonnode. Or, schema could be a pojo that is serialized itself. Or, schema could tie into json serialization in some other way that I can't see. What do you think?

It stinks to have so much schema knowledge spread out among serializers, but I guess the schema knowledge needs to be spread out over the serializers, or all that serializer context would be duplicated in schema anyway. I think letting serializers make the minimal schema, which can be manipulated as a pojo, is the most natural way to go. After you get the minimal schema, you can change it how ever you like before it is serialized. I think a basicbeandescription or annotatedclass should be kept in the schema object, in order to be query-able for schema customization.

The schema draft doesn't seem like it has completely matured, so making schema into its own project which can be independently versioned would make sense as a long term goal because I hope in the future the community will devise new schema versions. Serializers' implementations would become out of date if schema changes dramatically, but I don't see how this can be helped.

cowtowncoder · 2012-07-27T17:48:36Z

Hmhm. I don't know whether I like this or not.

On one hand, a properly typed schema structure is nice and makes sense.

But on the other hand, this will also grow the scope of what core jackson component knows about.

So I really really would like to figure out how to carve this out of jackson-databind, so that it would be an optional add-on part, since schema handling is not in my opinion something that belongs in core. The reason code is there is a historical leftover, as generation was added as early as Jackson 1.1 or so.

I don't know how exactly that should work, but I think first part would be adding a visitor style interface to JsonSerializer, so that it would be possible to traverse logical structure. Visitor interface(s) would be part of databind package, but implemented by jackson-module-schema (or schema-generator?) module.

bytesandwich · 2012-07-27T18:26:53Z

Maybe I can just refactor schemaAware into visitable, and have a schemafactory visitor make schema objects. I think schemas should just be pojos that can be serialized like any other pojo at the end of visiting.

cowtowncoder · 2012-07-27T18:35:47Z

That sounds like a good idea. And I agree in that Schema should be serializable as JSON, by default, to make some use of the fact that they are in JSON (which about the only benefit really :) ).

One concern is that change must be backwards compatible, to some degree. That is, old serializers should still work; although I think that some breakage of schema production (for custom types) is acceptable if it can't be avoided.
Just need to make sure that schema generation for standard types works, which is doable as it's all in databind package.

schemas are proper objects, except for hyperschema

bytesandwich · 2012-07-30T21:07:52Z

I made a tentative visitor/acceptor relationship for the format serializers produce. Schema is a proper pojo, and serializers return schemas.

I would remove serializableserializer and withdraw support for jsonserializableschema, because the literal input of the schema as a string is error prone and requires parsing in its own right.

if you look at
asarrayserializerbase.acceptjsonformatvisitor
I am not sure what a refactoring would entail. I should always be passing the typehint, so maybe I can just remove the other type lookups?

Is there any configured loggers for Jackson? Or is there any way for me to record exceptions that shouldn't be thrown but are thrown?

Currently I am debugging testGenerateJsonSchema.testGeneratingJsonSchema(),
because whether the mapper is MAPPER or new ObjectMapper() its serializerprovider has a null config at BeanSerializerBase isPropertyRequired while accepting a visitor for the first property to schema-ize.

bytesandwich · 2012-07-30T21:11:59Z

I'm on a branch
for some reason when I follow that link I need to refresh for styles to make sense.

as array

some access is formatted incorrectly or, less likely, there is a bug in jackson.

…aType, but some access is formatted incorrectly or, less likely, there is a bug in jackson. " This reverts commit 7fd884e.

ignored

bytesandwich · 2012-08-04T01:51:30Z

Ok I pass all tests.
Four nested class hierarchies in ArraySchema and ObjectSchema are not deserialized because I think #43 is exactly what they need.

I tentatively withdrew support for JsonSerializableSchema including jackson-813 because I left a hyperschema class skeleton, but I don't think Jackson should make any inroads to establishing web application structural commitments.
It wouldn't take more than a minute to replace them, but I hope you will consider letting them drop.

JsonSerializableSchema also requires that a passed Json String be parsed and used as the json schema representation for an object. At best I would have allowed specification of a schematype from teh eight types, and a list of strings for properties and their types, but including the literal string representation seems to be completely at odds with the point of using Jackson. What do you think?

Looking at my commits, the messages aren't very helpful. Sorry about that. I will improve that in the future.

bytesandwich · 2012-08-07T00:18:51Z

I think schema is pretty good. I think it could be a separate project if you you accept the format visitor changes into databind.

Would you mind glancing over the status of this pull request?

To be honest, though, I am not very well versed in Maven. I don't really know what it would entail to get a maven repo up. I just run the tests from maven for databind, and include the dependency from my databind eclipse project in my other projects. I wasn't able to get bundle:bundle (if that is even correct :P) to work.

I personally find gradle much more transparent, but I don't think it would makes sense to have two different build tools.

cowtowncoder · 2012-08-07T01:30:14Z

Thanks. I will check out visitor changes and give you feedback. Timing should be good, as changes could go in 2.1, and once that is released, the new project could depend on it.

To fully build jackson-databind, you can just do 'mvn install', and that should run things you need. There isn't that much magic. Although I usually run things via Eclipse during development, and then use direct maven build before checking things in (to guard against regressions etc).

cowtowncoder · 2012-08-07T01:51:55Z

I like the visitor pattern. I can not confirm whether it works well as abstraction layer (since it's been a while since I read the spec), but I trust you that it works as you have made the code work. Also, we can consider visitor design somewhat experimental at first (i.e. reserve the right to change it, even radically), so that it changes must be made between Jackson 2.1 and 2.2, this can be done.

Now, the only remaining thing (aside from some changes needed due to unrelated changes I made to enum handling -- I think I can not merge pull request automatically) is that of carving out of the external part.
If necessary I guess I could merge this request in, and we could then extract the new project out of that. Although ideally I would avoid checking in things that will go out. What do you think?

There is also need to create the new project. So what should it be called? jackson-json-schema? Or maybe it does not even need to have 'jackson' in it? I could create it under FasterXML and give you access, if you want; or, you can create it separately. Either way works fine. One benefit of using FasterXML would be visibility (i.e. slightly easier to find as part of "all jackson-related projects"), but that's not a big deal.

ObjectMapper now acceptsJsonFormatVisitor rather than generateJsonSchema. Added setProvider to JsonFormatVisitor so that ObjectMapper can pass reference to first visitor. Schema Hierarchy passes parent references, and gets provider from parent.

cowtowncoder · 2012-08-07T03:45:53Z

src/main/java/com/fasterxml/jackson/databind/ObjectMapper.java

@@ -2148,7 +2148,7 @@ public ObjectWriter writer(FilterProvider filterProvider) {
     * pass specific schema object to {@link JsonGenerator} used for
     * writing content.
     * 
-     * @param schema Schema to pass to generator
+     * @param schema JsonSchema to pass to generator


This is probably an accidental change: schema here does NOT refer to JSON Schema (but rather to thing like what CSV module needs -- unfortunate overload for sure).

- All jsonformat visitors now accept the javatype of the translated java type in their factory constructor call - organized imports to remove references to jsonnodes and such

Rather than SchemaFactory being an instantiable class, and thus mucking up its methods shared with subtypes, visitorwrapper -> factoryprovider provide access to schemafactory subtypes

(but rather to thing like what CSV module needs -- unfortunate overload for sure).

moved jsonschema.visitors to jsonFormatVisitors removed jsonSchema as a terminal package auto changed imports for all interested parties

bytesandwich · 2012-08-10T16:52:45Z

I think it makes more sense to keep it under FasterXML.

Separation should be pretty clean. com.fasterxml.jackson.databind.jsonschema.visitors should remain in data bind,

and the following:
com.fasterxml.jackson.databind.jsonschema.types
com.fasterxml.jackson.databind.jsonschema.factories
are the schema objects

the JsonSchema tests should also migrate

I didn't change the pom to reflect the changed packages.
I would think to just call it jsonschema and let it be com.fasterxml.jackson.jsonschema

bytesandwich · 2012-08-10T16:53:19Z

I renamed com.fasterxml.jackson.databind.jsonschema.visitors to
com.fasterxml.jackson.databind.jsonFormatVisitors

cowtowncoder · 2012-08-10T17:41:52Z

Thanks. The only thing I have to think is the project naming -- I prefer using a prefix like "jackson-module-jsonschema" just for primitive namespacing (and similarly for java packages). Not a big deal, but helps manage things bit better.
But I don't know if this would necessarily be a module; probably not, and it'd be more like separate library to use, even if relying on Jackson for parsing.

bytesandwich · 2012-08-10T17:54:41Z

Being consistent with previous naming is probably most important.

Unrelated: I think this version of data-bind breaks hibernate-module? I cloned hibernate module and running tests I got https://gist.github.com/3316148

cowtowncoder · 2012-08-10T19:10:51Z

Ok, it is definitely something to solve. I can try to verify that it was not already broken as well; I have not yet upgrade hibernate master to use jackson 2.1

bytesandwich · 2012-08-10T19:35:42Z

Actually, the test just failed for me on a fresh clone.
FasterXML/jackson-datatype-hibernate#18

running against 2.1.0-SNAPSHOT, they still fail.
possibly I have some error in my maven configuration.

cowtowncoder · 2012-08-10T20:19:31Z

FWIW, I think this is for issue #15 at Hib(4) module: FasterXML/jackson-datatype-hibernate#15

so it is not caused by this patch. But thanks for pointing it out; I need to figure out how the other part is to be resolved.

cowtowncoder · 2012-08-11T17:00:39Z

Hmmh. Actually, looks like I can't automatically merge the pull request for some reason. Possibly due to release of 2.0.5 version, which changed pom, and for which I integrate one fix from 'master'.

Can you merge this with latest from hibernate3?

Mostly updated imports and writes on old schema parsing Conflicts: src/main/java/com/fasterxml/jackson/databind/ObjectMapper.java src/main/java/com/fasterxml/jackson/databind/ser/impl/SimpleBeanPropertyFilter.java src/main/java/com/fasterxml/jackson/databind/ser/std/BeanSerializerBase.java src/main/java/com/fasterxml/jackson/databind/ser/std/EnumSerializer.java

bytesandwich · 2012-08-12T00:59:41Z

tests all passed for me once I updated my repo version of core and annotations. I haven't mastered git rebasing, so I just merged.

cowtowncoder · 2012-08-12T02:43:01Z

Ok -- I will just merge this, and we can then cleave stuff out to the new project next.

cowtowncoder · 2012-08-13T23:40:42Z

I should have reviewed this more carefully; I just rolled it back.

The main problem is that it changes existing public API, by renaming 'generateJsonSchema()' (or, renaming and changing).
While that's what we want to do eventually, it is not something that can be done for 2.1 (or in theory for any 2.x).
Rather, new functionality has to co-exist; so that new visit methods will be added, and old functionality is deprecated, including generation methods. Once it is determined that there is an upgrade path, methods may be removed: ideally this would only be done for 3.0, but if we feel confident after couple of minor releases that users can move on, we may be able to remove them earlier.

So, what is needed is keeping old generation mechanism in some form, and add new improved forms. It may not be possible to share much code: that is acceptable (if not ideal).

bytesandwich · 2012-08-13T23:46:33Z

I think generatejsonschema can use the visitors behind the scenes and just make the same jsonschema it had before, with the additional ability to produce the java json scheme that I added.

I still recommend removing jsonserializableschema, simply because it indicates the json representation for the schema as a manually inputted string.

cowtowncoder · 2012-08-14T03:57:00Z

Yes, technically we could make old API use the new implementation. But the problem there is conflict between the goal of moving functionality out of core.

There are at least two ways to go about that:

Indicate that although on short term (2.1) all functionality would be included; but later on, support would require inclusion of additional module, or
Just keep pieces separate, so that new one would include new module, and not go through ObjectMapper; and then deprecate old part.

The reason I think I prefer (2) is just so that new functionality can start completely clean, and question of developing new approach is separate from that of deprecating old system. That is, let the old part die out semi-naturally, with deprecation and so on.

As to JsonSerializableSchema: I think it need not be supported for new code; however, existing application code is likely to use it. That is, there are users who expect it to work, via current API. So I think it needs to continue work with old API, as long as that API itself works. But no further.

I understand that it is an ugly fallback, and not really compatible with better system.

Does this make sense?

bytesandwich · 2012-08-14T08:29:21Z

I just saw these two emails after I wrote you to suggest changing the
implementation of generateSchema inside objectmapper to a jsonNode visitor.
I think that my proposed change could be minimized so really only the
SchemaAware interface inside serializers is changing, but if change needs
to be more gradual, I understand.

-Jack

On Mon, Aug 13, 2012 at 11:57 PM, Tatu Saloranta
[email protected]:

Yes, technically we could make old API use the new implementation. But the
problem there is conflict between the goal of moving functionality out of
core.

There are at least two ways to go about that:

Indicate that although on short term (2.1) all functionality would
be included; but later on, support would require inclusion of additional
module, or

Just keep pieces separate, so that new one would include new
module, and not go through ObjectMapper; and then deprecate old part.

The reason I think I prefer (2) is just so that new functionality can
start completely clean, and question of developing new approach is separate
from that of deprecating old system. That is, let the old part die out
semi-naturally, with deprecation and so on.

As to JsonSerializableSchema: I think it need not be supported for new
code; however, existing application code is likely to use it. That is,
there are users who expect it to work, via current API. So I think it needs
to continue work with old API, as long as that API itself works. But no
further.

I understand that it is an ugly fallback, and not really compatible with
better system.

Does this make sense?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/39#issuecomment-7715606.

Fix two FindBugs items

tentative json schema java representation.

b9c9d01

serializers return schema pojos (untested)

8cbae7b

schemas are proper objects, except for hyperschema

jackphel added 8 commits July 30, 2012 17:42

replaced getClass with getRawClass

b113cd6

Generates schema in TestGenerateJsonSchema without throwing errors

5b0fd4d

working, but passed type information isn't precise enough for collection

65e5dda

as array

jsonFormatVisitor/acceptor now passes/accepts typeHint of JavaType, but

7fd884e

some access is formatted incorrectly or, less likely, there is a bug in jackson.

Revert "jsonFormatVisitor/acceptor now passes/accepts typeHint of Jav…

c4db6f2

…aType, but some access is formatted incorrectly or, less likely, there is a bug in jackson. " This reverts commit 7fd884e.

working with JavaType

cee882b

working schema serialization, except for JsonSerializableSchema is

8fababd

ignored

jsonschema serialization and deep equals implemented.

ed98c9e

continuous loop property filter checking into separate loops.

91c4467

cowtowncoder reviewed Aug 7, 2012
View reviewed changes

jackphel added 5 commits August 7, 2012 13:54

Added javatype reference to all factories, organized imports

7e50672

- All jsonformat visitors now accept the javatype of the translated java type in their factory constructor call - organized imports to remove references to jsonnodes and such

adds new visitor wrapper, which constructs typed visitors

8e8c6e3

Rather than SchemaFactory being an instantiable class, and thus mucking up its methods shared with subtypes, visitorwrapper -> factoryprovider provide access to schemafactory subtypes

factored getSchema into SchemaFactory

c11852f

schema here does NOT refer to JSON Schema

6288e8b

(but rather to thing like what CSV module needs -- unfortunate overload for sure).

removed unused constructor in arrayschemafactory

248ab7e

refactored packages for separation

8f665dd

moved jsonschema.visitors to jsonFormatVisitors removed jsonSchema as a terminal package auto changed imports for all interested parties

Needed to change packages in pom to run maven install

f46d555

cowtowncoder merged this pull request into FasterXML:master Aug 12, 2012

christophercurrie pushed a commit to christophercurrie/jackson-databind that referenced this pull request Jul 19, 2015

Merge pull request FasterXML#39 from fge/master

b47bc9f

Fix two FindBugs items

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tentative development for #38 #39

tentative development for #38 #39

bytesandwich commented Jul 27, 2012

bytesandwich commented Jul 27, 2012

bytesandwich commented Jul 27, 2012

bytesandwich commented Jul 27, 2012

cowtowncoder commented Jul 27, 2012

bytesandwich commented Jul 27, 2012

cowtowncoder commented Jul 27, 2012

bytesandwich commented Jul 30, 2012

bytesandwich commented Jul 30, 2012

bytesandwich commented Aug 4, 2012

bytesandwich commented Aug 7, 2012

cowtowncoder commented Aug 7, 2012

cowtowncoder commented Aug 7, 2012

cowtowncoder Aug 7, 2012

bytesandwich commented Aug 10, 2012

bytesandwich commented Aug 10, 2012

cowtowncoder commented Aug 10, 2012

bytesandwich commented Aug 10, 2012

cowtowncoder commented Aug 10, 2012

bytesandwich commented Aug 10, 2012

cowtowncoder commented Aug 10, 2012

cowtowncoder commented Aug 11, 2012

bytesandwich commented Aug 12, 2012

cowtowncoder commented Aug 12, 2012

cowtowncoder commented Aug 13, 2012

bytesandwich commented Aug 13, 2012

cowtowncoder commented Aug 14, 2012

bytesandwich commented Aug 14, 2012

tentative development for #38 #39

tentative development for #38 #39

Conversation

bytesandwich commented Jul 27, 2012

bytesandwich commented Jul 27, 2012

bytesandwich commented Jul 27, 2012

bytesandwich commented Jul 27, 2012

cowtowncoder commented Jul 27, 2012

bytesandwich commented Jul 27, 2012

cowtowncoder commented Jul 27, 2012

bytesandwich commented Jul 30, 2012

bytesandwich commented Jul 30, 2012

bytesandwich commented Aug 4, 2012

bytesandwich commented Aug 7, 2012

cowtowncoder commented Aug 7, 2012

cowtowncoder commented Aug 7, 2012

cowtowncoder Aug 7, 2012

Choose a reason for hiding this comment

bytesandwich commented Aug 10, 2012

bytesandwich commented Aug 10, 2012

cowtowncoder commented Aug 10, 2012

bytesandwich commented Aug 10, 2012

cowtowncoder commented Aug 10, 2012

bytesandwich commented Aug 10, 2012

cowtowncoder commented Aug 10, 2012

cowtowncoder commented Aug 11, 2012

bytesandwich commented Aug 12, 2012

cowtowncoder commented Aug 12, 2012

cowtowncoder commented Aug 13, 2012

bytesandwich commented Aug 13, 2012

cowtowncoder commented Aug 14, 2012

bytesandwich commented Aug 14, 2012