nextcloud / tables

🍱 Nextcloud tables app
https://apps.nextcloud.com/apps/tables
GNU Affero General Public License v3.0
142 stars 24 forks source link

fix(API): declared array shape was not correct #1169

Closed blizzz closed 3 months ago

blizzz commented 3 months ago

fixes #1157

blizzz commented 3 months ago

/backport to stable0.7

provokateurin commented 2 months ago

This fix is not correct, it now requires a list with and two elements of int and mixed https://psalm.dev/r/74104d149c. You can also see that the generated spec is totally wrong and invalid. It should have been array<int, mixed> to achieve what you want.

blizzz commented 2 months ago

This fix is not correct, it now requires a list with and two elements of int and mixed https://psalm.dev/r/74104d149c. You can also see that the generated spec is totally wrong and invalid. It should have been array<int, mixed> to achieve what you want.

Thank you for your direct reply. You are right of course.

The odd thing is, that OpenAPI will not build with the correct pairs of brackets:

PHP Fatal error:  Uncaught Error: api1#createRowInTable: Unable to resolve OpenAPI type:
\PHPStan\PhpDocParser\Ast\Type\GenericTypeNode::__set_state(array(
   'type' => 
  \PHPStan\PhpDocParser\Ast\Type\IdentifierTypeNode::__set_state(array(
     'name' => 'array',
     'attributes' => 
    array (
    ),
  )),
   'genericTypes' => 
  array (
    0 => 
    \PHPStan\PhpDocParser\Ast\Type\IdentifierTypeNode::__set_state(array(
       'name' => 'int',
       'attributes' => 
      array (
      ),
    )),
    1 => 
    \PHPStan\PhpDocParser\Ast\Type\IdentifierTypeNode::__set_state(array(
       'name' => 'mixed',
       'attributes' => 
      array (
      ),
    )),
  ),
   'variances' => 
  array (
    0 => 'invariant',
    1 => 'invariant',
  ),
   'attributes' => 
  array (
  ),
))

a plain array<mixed> will do, apart of a general error (✘ Globbing has been deprecated in favor of redocly.yaml’s apis keys.) that also appears without a change. The IDE complains, however, and suggests to replace with array, which will openapi enrage, because it might be empty :roll_eyes:

provokateurin commented 2 months ago

array<mixed> shouldn't be used either. I will look into the error and then hopefully provide a fix here as well.

blizzz commented 2 months ago

array<mixed> shouldn't be used either. I will look into the error and then hopefully provide a fix here as well.

No, that was a test just for scientific reasons.

provokateurin commented 2 months ago

It doesn't work because this is impossible in JSON. There is no way to index an object by non-strings, so it has to be a list or the keys need to be changed to strings. Since you cast to int later anyway, changing to string seems fine? I'll add a better warning to openapi-extractor for these cases.