Skip to content

9 endless recursion fix#10

Open
Haniyya wants to merge 8 commits into
parrish:masterfrom
Haniyya:9-endless-recursion-fix
Open

9 endless recursion fix#10
Haniyya wants to merge 8 commits into
parrish:masterfrom
Haniyya:9-endless-recursion-fix

Conversation

@Haniyya

@Haniyya Haniyya commented Mar 5, 2019

Copy link
Copy Markdown

Fixes #9

This fixes the endless recursion happening on extending objects with null: true.

The problem was that, when extending, objects get reinitialized. In the extract_type they were then further extended using any_of(null) which caused an infinite loop.

Another problem was that adding children to nullable entities could lead to the childrens properties getting merged not into the any_of structure, but the root structure, which was not desired. I added a test and and a build_any_of call to Object#initialize_children.

I've moved the any_of(null) call to initialization (the normal one) so that this does not happen.

I also added a specification of what I think the resulting schema should look like and had to do horrible hacks to get there. I aim to clean those up shortly, thus the.

Haniyya added 2 commits March 5, 2019 10:38
true.

Added that object to the specifications.
avoid endless recursion.

Also added a horrible, disgusting hack to extend the initial object
option (option as in possible anyOf value) by newly added attributes.
This does look disguting and is probably unstable for a lot of cases.
Comment thread lib/json/schema_builder/entity.rb Outdated
everything_else = schema.data.reject { |k, v| k == "anyOf" }
return unless everything_else.present?
schema.data.select! { |k, v| k == "anyOf" }
if initial = schema.data['anyOf'].find { |opt| opt.as_json.try(:[], 'type') == 'object' }

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that, when some is extending this, they intend to extend the object, not the null, so I extract the initial definition and extend it here. This is really ugly right now though and probably won't work in some cases. Happy for any comments here.

@stex

stex commented Mar 5, 2019

Copy link
Copy Markdown

@Haniyya Thanks a lot for your work here!

This fixes the problem with the endless loop, but introduced a different problem as it seems that the properties of an object with null: true (or probably simply an anyOf?) are duplicated outside the anyOf.

The following code generates a wrong schema:

def schema
  object.tap do |base_obj|
    foo_obj = base_obj.object(:foo, required: true)
    bar_obj = foo_obj.object(:bar, null: true, required: true)
    bar_obj.string :baz
  end
end
{
  "type": "object",
  "properties": {
    "foo": {
      "type": "object",
      "properties": {
        "bar": {
          "anyOf": [
            {
              "type": "object",
              "properties": {
                "baz": {
                  "type": "string"
                }
              }
            },
            {
              "type": "null"
            }
          ],
          "properties": {
            "baz": {
              "type": "string"
            }
          }
        }
      },
      "required": [
        "bar"
      ]
    }
  },
  "required": [
    "foo"
  ]
}

Writing the same schema using nesting instead of extending, it looks correct:

def expected_schema
  object do
    object :foo, required: true do
      object :bar, null: true, required: true do
        string :baz
      end
    end
  end
end
{
  "type": "object",
  "required": [
    "foo"
  ],
  "properties": {
    "foo": {
      "type": "object",
      "required": [
        "bar"
      ],
      "properties": {
        "bar": {
          "anyOf": [
            {
              "type": "object",
              "properties": {
                "baz": {
                  "type": "string"
                }
              }
            },
            {
              "type": "null"
            }
          ]
        }
      }
    }
  }
}

@Haniyya

Haniyya commented Mar 6, 2019

Copy link
Copy Markdown
Author

@stex Should be fixed. Issue was that Object#initialize_children was called after the any_of construction was done, so the properties were added to the main object.

@Haniyya Haniyya changed the title WIP: 9 endless recursion fix 9 endless recursion fix Mar 6, 2019
@stex

stex commented Mar 7, 2019

Copy link
Copy Markdown

@parrish Could you take a look at the changes here and possibly release a new gem version if you have time? That would help a lot :)

@stex

stex commented Mar 7, 2019

Copy link
Copy Markdown

@Haniyya I found another problem with this PR: When using nested "nullable" anyOfs, the generated schema contains a type as well as the anyOf. This causes the schema validation to fail as type: 'string' is more important than the anyOf:

def schema
  object.tap do |base_obj|
    obj = base_obj.object :nullable_object, null: true
    obj.string :nullable_string, null: true
  end
end
{
  "type": "object",
  "properties": {
    "nullable_object": {
      "anyOf": [
        {
          "type": "object",
          "properties": {
            "nullable_string": {
              "type": "string",
              "anyOf": [
                {
                  "type": "string"
                },
                {
                  "type": "null"
                }
              ]
            }
          }
        },
        {
          "type": "null"
        }
      ]
    }
  }
}

def expected_schema
  object do
    object :nullable_object, null: true do
      string :nullable_string, null: true
    end
  end
end
{
  "type": "object",
  "properties": {
    "nullable_object": {
      "anyOf": [
        {
          "type": "object",
          "properties": {
            "nullable_string": {
              "anyOf": [
                {
                  "type": "string"
                },
                {
                  "type": "null"
                }
              ]
            }
          }
        },
        {
          "type": "null"
        }
      ]
    }
  }
}

@Haniyya

Haniyya commented Apr 1, 2019

Copy link
Copy Markdown
Author

@stex Added a horrible fix. I have a better solution in mind but that would require quite a bit of refactoring work:

  1. Make new entities out of any/all/one_of attributes
  2. Have Nullable be a specialization of any_of that proxies all calls from the DSL module to it's initial object
  3. ???
  4. Profit.

So I basically could do this nicely, but It would take a lot of rework. For now this should work.

@Haniyya Haniyya closed this Apr 1, 2019
@Haniyya Haniyya reopened this Apr 1, 2019
@Haniyya

Haniyya commented Apr 1, 2019

Copy link
Copy Markdown
Author

Accidentally closed this. Not used to github.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extending objects with null: true causes a StackLevelTooDeep exception

2 participants