[JsonSchema] Use arrayObject for objects

[Rid]

In order to fix #700, when we initialise an array, we first check if it should be a simple array or an arrayObject by checking the object type.

Further, when initialising variables in sub statements (for loops) which should be objects, we create arrayObjects.

example spec:

"PortBindings": {
                "$ref": "#/components/schemas/PortMap"
              },

"PortMap": {
        "type": "object",
        "additionalProperties": {
          "type": "array",
          "nullable": true,
          "items": {
            "$ref": "#/components/schemas/PortBinding"
          }
        },
        "description": "PortMap describes the mapping of container ports to host ports, using the\ncontainer's port-number and protocol as key in the format `<port>/<protocol>`,\nfor example, `80/udp`.\n\nIf a container's port is mapped for multiple protocols, separate entries\nare added to the mapping table.\n",
        "example": {
          "443/tcp": [
            {
              "HostIp": "127.0.0.1",
              "HostPort": "4443"
            }
          ],
          "80/tcp": [
            {
              "HostIp": "0.0.0.0",
              "HostPort": "80"
            },
            {
              "HostIp": "0.0.0.0",
              "HostPort": "8080"
            }
          ],
          "80/udp": [
            {
              "HostIp": "0.0.0.0",
              "HostPort": "80"
            }
          ],
          "53/udp": [
            {
              "HostIp": "0.0.0.0",
              "HostPort": "53"
            }
          ]
        }
      },
      "PortBinding": {
        "type": "object",
        "properties": {
          "HostIp": {
            "type": "string",
            "description": "Host IP address that the container's port is mapped to.",
            "example": "127.0.0.1"
          },
          "HostPort": {
            "type": "string",
            "description": "Host port number that the container's port is mapped to.",
            "example": "4443"
          }
        },
        "description": "PortBinding represents a binding between a host IP address and a host\nport.\n"
      },

Old output:

        if ($object->isInitialized('portBindings') && null !== $object->getPortBindings()) {
            $values_10 = array();
            foreach ($object->getPortBindings() as $key => $value_10) {
                $values_11 = array();
                foreach ($value_10 as $value_11) {
                    $values_11[] = $this->normalizer->normalize($value_11, 'json', $context);
                }
                $values_10[$key] = $values_11;
            }
            $data['PortBindings'] = $values_10;
        }

New output:

        if ($object->isInitialized('portBindings') && null !== $object->getPortBindings()) {
            $values_10 = new \ArrayObject(array(), \ArrayObject::ARRAY_AS_PROPS);
            foreach ($object->getPortBindings() as $key => $value_10) {
                $values_11 = array();
                foreach ($value_10 as $value_11) {
                    $values_11[] = new \ArrayObject($this->normalizer->normalize($value_11, 'json', $context), \ArrayObject::ARRAY_AS_PROPS);
                }
                $values_10[$key] = $values_11;
            }
            $data['PortBindings'] = $values_10;
        }

[Rid]

It looks like the tests are also outputting incorrect code.

@Korbeil Can you take a look at the fix, if you think it looks good, I can update the tests.

Just an aside: I'm currently using the fix in production on the docker API and it's working well.

[Korbeil]

I didn't understood you wanted a review on the code before the commend you made today on the issue.
On the fly it seems totally ok to me, also I saw you already generated the docker-php client with and it seems it does fix your issue so I'm all for it 👍

[Rid]

Thanks @Korbeil, I'll fix the tests this or next week and then you can merge.

[achton]

Thanks for this patch.

A bit of feedback: This patch fixes the case where Jane emits an empty array instead an empty object, but it introduces issues with other types, ie. nullable objects with local properties like:

        caseAgent:
          type: object
          properties:
            name:
              type: string
              description: The name of the agent responsible for the case.
            phoneNumber:
              type: string
              description: A phone number for contacting the agent.
          required:
            - name
            - phoneNumber
          nullable: true

This changes the normalizer code to:

$data['caseAgent'] = new \ArrayObject($this->normalizer->normalize($object->getCaseAgent(), 'json', $context), \ArrayObject::ARRAY_AS_PROPS);

and causes a fatal error when the value is null: InvalidArgumentException: Passed variable is not an array or object in ArrayObject->__construct().

This may be the cause of the failing tests, in which case you can ignore me :-)

[Korbeil]

Hey, any news about fixing tests ? :)

[Rid]

@Korbeil I've fixed the tests, but it does not resolve @achton's issue.

We should check if the value is null at runtime and pass it as null instead of an object.

[Rid]

Here's what it might look like if we test for null:

    /**
     * @return array|string|int|float|bool|\ArrayObject|null
     */
    public function normalize($object, $format = null, array $context = array())
    {
        $data = array();
        if ($object->isInitialized('foo') && null !== $object->getFoo()) {
            $data['foo'] = $object->getFoo();
        }
        if ($object->isInitialized('bar') && null !== $object->getBar()) {
            $data['Bar'] = $object->getBar() == null ? null : new \ArrayObject($this->normalizer->normalize($object->getBar(), 'json', $context), \ArrayObject::ARRAY_AS_PROPS);
        }
        if ($object->isInitialized('baz') && null !== $object->getBaz()) {
            $data['Baz'] = $object->getBaz() == null ? null : new \ArrayObject($this->normalizer->normalize($object->getBaz(), 'json', $context), \ArrayObject::ARRAY_AS_PROPS);
        }
        foreach ($object as $key => $value) {
            if (preg_match('/.*/', (string) $key)) {
                $data[$key] = $value;
            }
        }
        return $data;
    }

It does a double check if we're using OpenAPIv3 and nullable is not true on the object and the object isn't null, it makes more sense for OpenAPIv2 and OpenAPIv3 where nullable is true, I think it does cover all schemas.

@Korbeil Let me know what you think?

[Rid]

@Korbeil I've updated to handle the above and fixed the tests. If you agree with the changes I will squash the commits.

[Korbeil]

Hey @Rid ,
Thanks for all the work on this PR !

You proposal for nullable handling seems totally fine to me.
The code seems fine to me :)

[Korbeil]

✔️

[Korbeil]

Could you add a note in the CHANGELOG about this change ? 🙏

[Rid]

@Korbeil I've updated the CHANGELOG and squashed the commits into one.

This closes:

#700
#680

[Rid]

Ping @Korbeil

[thePanz]

@Rid looks like you need to rebase and solve the conflicts on this PR

[Rid]

@thePanz happy to rebase, do you have merge permissions?

[thePanz]

@thePanz happy to rebase, do you have merge permissions?

no I don't, but this PR is marked as not mergeable by github due to merge-conflicts