Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No CRUD generation support for ARRAY types. #109

Closed
gmos opened this issue Nov 30, 2021 · 15 comments · Fixed by #115
Closed

No CRUD generation support for ARRAY types. #109

gmos opened this issue Nov 30, 2021 · 15 comments · Fixed by #115
Labels
bug Something isn't working
Projects

Comments

@gmos
Copy link

gmos commented Nov 30, 2021

At version 0.29.0 there seems to be no support for ARRAY types in the CRUD generation.
Code is generated, but an ARRAY of VARCHAR comes out as a single Varchar, and the field cannot be properly used.

In the Piccolo Admin the Arrays of Varchar are handled ok. Are there already plans to support Array types in CRUD?

@gmos gmos changed the title No CRUD support for ARRAY types. No CRUD generation support for ARRAY types. Nov 30, 2021
@dantownsend
Copy link
Member

@gmos Thanks for reporting this.

PiccoloCRUD should support arrays - there could be a bug somewhere.

In the example below, the years_nominated field is an array. The API should render it like a list.

Screenshot 2021-12-02 at 10 05 43

Is the problem when trying to post a new value back?

@sinisaos
Copy link
Member

sinisaos commented Dec 2, 2021

@gmos @dantownsend POST also work as expected.

post

@dantownsend
Copy link
Member

@sinisaos Thanks for checking that.

@gmos
Copy link
Author

gmos commented Dec 2, 2021

Screenshots are from the swagger .../docs page. I expanded the root GET.

Parameters docs:
image
Here it goes wrong. No array structure to be found. The sensors field is depicted as a simple Varchar.

And this is when I make the actual GET call:
image

As you can see the response schema is good. the issue is about the generated parameter input for the root GET.

@dantownsend
Copy link
Member

@gmos Cool, thanks - that's really useful. We'll investigate.

@gmos
Copy link
Author

gmos commented Dec 2, 2021

Here is the column definition from the Table subclass:

    sensors = Array(
        base_column=Varchar(
            length=16,
            null=False,
        )
    )

And here the automatic creation of the lot.

hoses_router = APIRouter()
hoses_crud = PiccoloCRUD(TempHoses, read_only=False)

FastAPIWrapper(
    "/hoses",
    fastapi_app=cast(FastAPI, hoses_router),  # TODO Get away with the cast
    piccolo_crud=hoses_crud,
    fastapi_kwargs=FastAPIKwargs(
        all_routes={"tags": ["TempHoses"]},
    ),
)

Side question: why do I need a cast to get rid of the type warning? AM I doing something wrong assigning a PiccoloCRUD instance to the FastAPIwrapper?

@dantownsend
Copy link
Member

dantownsend commented Dec 2, 2021

@gmos The type error might be because you're using FastAPIWrapper with an APIRouter instance, rather than a FastAPI instance.

We probably just need to update this type annotation to be t.Union[FastAPI, APIRouter]:

fastapi_app: FastAPI,

@sinisaos
Copy link
Member

sinisaos commented Dec 2, 2021

@dantownsend Problem is when you use openapi request url is http://localhost:8000/posts/?tags=tag (which raise Pydantic ValidationError value is not a valid list (type=type_error.list)) and it should be http://localhost:8000/posts/?tags[]=tag like in Piccolo Admin or if you pass request url directly in browser to get correct result.

@dantownsend
Copy link
Member

@sinisaos Well remembered - I forgot about that.

I wonder if we can add a custom validator to the Pydantic model:

https://pydantic-docs.helpmanual.io/usage/validators/

@sinisaos
Copy link
Member

sinisaos commented Dec 9, 2021

@dantownsend I think I found a problem in openapi filters. We need to use ModeField.outer_type_

type_ = _field.type_
because ModeField.type_ shows the wrong type (class int or str insted List[int] or List[str]).
We need to change the line 401 to this type_ = _field.outer_type_ and the result is.

arrays

Then I add an extra check in crud endpoints _parse_params method for the array columns so that openapi request url accepts multiple query params without square brackets. Method now looks like this

def _parse_params(self, params: QueryParams) -> t.Dict[str, t.Any]:
  params_map: t.Dict[str, t.Any] = {
      i[0]: [j[1] for j in i[1]]
      for i in itertools.groupby(params.multi_items(), lambda x: x[0])
  }

  array_columns = [
      i._meta.name
      for i in self.table._meta.columns
      if i.value_type == list
  ]

  output = {}

  for key, value in params_map.items():
      if key.endswith("[]") or key.rstrip("[]") in array_columns:
          # Is either an array, or multiple values have been passed in
          # for another field.
          key = key.rstrip("[]")
      elif len(value) == 1:
          value = value[0]

      output[key] = value

  return output

And result is

query

Piccolo Admin and request directly in browser remain unchanged.

@gmos Can you please check this on your local Piccolo Api installation and confirm that everything works.?

@dantownsend
Copy link
Member

@sinisaos That's some good investigative work - thanks.

When we make this change we'll also have to update Piccolo Admin at the same time.

@dantownsend
Copy link
Member

@sinisaos Ah, just realised you managed to make it compatible with the current Piccolo Admin, great.

@sinisaos
Copy link
Member

sinisaos commented Dec 9, 2021

@dantownsend Yes, Piccolo Admin works. If you want I can do PR.

@dantownsend
Copy link
Member

@sinisaos That would be great, thanks

@dantownsend
Copy link
Member

I've released it to PyPI now - version 0.29.2.

Seems to fix the problem. Thanks both.

@dantownsend dantownsend added this to To do in Bugs via automation Dec 9, 2021
@dantownsend dantownsend moved this from To do to Done in Bugs Dec 9, 2021
@dantownsend dantownsend added the bug Something isn't working label Dec 9, 2021
@dantownsend dantownsend linked a pull request Dec 9, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Bugs
Done
Development

Successfully merging a pull request may close this issue.

3 participants