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

models with ID columns not named ID cause exception #29

Closed
FalseProtagonist opened this issue Sep 11, 2017 · 3 comments
Closed

models with ID columns not named ID cause exception #29

FalseProtagonist opened this issue Sep 11, 2017 · 3 comments

Comments

@FalseProtagonist
Copy link

FalseProtagonist commented Sep 11, 2017

With a model that has an ID column not named ID, and the following code:

class TestView(PandasView):
    queryset = WeirdIDModel.objects.all()
    serializer_class = WeirdIDModelSerializer

when trying to query the class, you get an error KeyError, "id".

Reading that stack trace and the source I see that this is because by default we run "set_index('id') on the df, and this can be fixed in three obvious ways:

  1. set pandas_index on the meta of the serializer
  2. override get_index_fields on the serializer
  3. override get_dataframe
  1. setting the index seems to be needless duplication (I've already written once what the index of this model is)
  2. an override to get_index_fields I think would just create behaviour that should be default:
    WeirdIDModelSerializer._meta.pk.name gives the ID, for my version of Django at least

so something like

try:
    id_name = (self.<field for model>)._meta.pk.name
except:
    id_name = 'id'
default_fields = [id_name]
  1. Thinking about it more, and reading the source, it seems odd to even set an index, especially without preserving the original column - I think that DRF classes that we're mimicking would by default pass the index through to the client, and since I'm trying to drop-in replace some existing code which might rely on that, I'll have to replicate that manually.
@sheppard
Copy link
Member

Does a9f33fe fix this for you? It's in the master branch but not on PyPI yet.

@FalseProtagonist
Copy link
Author

It does, but it seems like a strange fix to me - it ties whether your pk is named "id" to the default indexing behavior of the serializer, which seem like unrelated concerns?

I'd prefer it to use metadata to work out the name of the primary key and then use that, rather than just looking for ID.

I don't know the code well enough but maybe default behavior of not setting an index at all would be better, like I said this breaks compatibility with DRF because that passes pk/id columns through to the user if fields aren't restricted.

Thanks for the quick reply, sorry I missed the existing issue.

@sheppard
Copy link
Member

Ok, that makes sense. I changed the default index to use model._meta.pk.name instead of "id".

The indexes not showing up in JSON output is an unintended side effect of using orient="records" with pandas' to_json() function. The index is useful for almost every other format, for example the Excel output which puts the index as the first column and bold. There isn't a JSON orient that does exactly what we want, but there is an open ticket to add one (pandas-dev/pandas#5729).

For now, I came up with a custom DRP-specific orient, "records-index", which resets the index before rendering the dataframe. I made this the default.

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

No branches or pull requests

2 participants