NODE_CLASS is overwritten when using a custom SchemaBuilder

[bvanelli]

First of all, I know that this is not the situation to open this issue, but I came across a situation where I wanted to provide a custom NODE_CLASS to the GenSON Schema parser. Let's simplify:

class ExampleNode(SchemaNode):
    def add_object(self, obj):
        super().add_object(obj)
        print("Hello world")

class ExampleSchemaBuilder(SchemaBuilder):
    NODE_CLASS = ExampleNode

I noticed that the print wasn't called at all. Digging deeper, the reason for it is that the meta-class directly references the class SchemaNode, while it should instead use cls.NODE_CLASS.

GenSON/genson/schema/builder.py

Lines 25 to 26 in 723f06e

	cls.NODE_CLASS = type('%sSchemaNode' % name, (SchemaNode,),
	{'STRATEGIES': cls.STRATEGIES})

Replacing it by cls.NODE_CLASS instead of SchemaNode for the base class solves the issue.

I'm willing to contribute the change if you think this is a relevant issue.

Have a great day!

[wolverdude]

Custom SchemaNode classes are not supported. In fact, the SchemaNode class itself should probably be private.

I'm curious about your use-case though. I know that SchemaStrategy classes may not be sufficient for some use-cases, but no one has brought that up yet, so I haven't addressed it. Could you say more about what you're attempting to do?

[bvanelli]

I was attempting to include a modification on the way the strategies were handled to add custom fields to the output response.

But since for all the strategies the modification was going to be the same, I realized it would be best to instead alter the NODE_CLASS, since it's the one that was always called on the top level before all the strategies. This was to avoid code repetition.

Then I tried to figure out why the custom NODE_CLASS was not working and ended up finding this assignment above.

Needless to say SchemaStrategy WAS enough for my use case, I was just wondering if I could do better 😆

You can close the issue or I can still provide the patch if you want to merge it. To me either way the NODE_CLASS needs to be made private with an underscore to make it clear that it's a private method or the meta-class can use the cls.NODE_CLASS instead of replacing it. I still find that the latter is more preferable, because there are no breaking changes and it makes the NODE_CLASS modding available for future hackers 😄

What do you think?

[wolverdude]

Thanks for the extra info.

Yes, I don't think I was careful enough about using underscore prefixes. I'll make a separate issue around that.

That said, I don't think I'm done with this issue yet. What custom fields specifically were you adding? Are they official JSON-Schema keywords? Perhaps they're listed here? If they are, we could look into what it would take to add built-in support for them.

One more thought: it might be possible to achieve the same thing by creating a custom base SchemaStrategy and then applying it as an ancestor to all the existing ones using multiple inheritance, but it's quite possible that's more brittle than changing the NODE_CLASS.

[bvanelli]

Hello,

Here is what I was trying to do, maybe you understand a bit better: brokenloop/jsontopydantic#16

I understand I'm not using the library as it was supposed to be used, since I'm trying to include a field that is not included on the standard.

One more thought: it might be possible to achieve the same thing by creating a custom base SchemaStrategy

Yeah, I tried that too, but I got a bit more complicated since the strategies have different bases. I ended up with the solution that looked more readable.

[wolverdude]

Hmm... I think it makes sense to have some way to make a simple change (like adding a field) to all strategies without using a ton of code. That's a customization I hadn't considered previously, but it's a very reasonable use-case. Custom SchemaStrategies were added to let people extend for non-canonical use-cases. There's no reason why this use-case should be different other than it's a little more technically challenging.

I'll create a new issue that encapsulates this problem more accurately and do some thinking on how to address it.