ordering direction

[Incanus3]

Hi,
I know this isn't the right place for Q&A, but I haven't found any documented place to ask questions, so I'm asking here. Is there a way to dynamically sort records in descending order, e.g. something like /clients?sort[]=name.desc&page=2&per_page=5 (this doesn't work obviously). I could sort the records on the client side, but that doesn't cover the pagination case.

[aleontiev]

I'm not quite sure what you mean -- can you give me an example as a Django queryset / Python code? If its possible to translate your request into a single queryset, then we should be able to support it.

[Incanus3]

When looking up the QuerySet documentation to achieve this, I found out you can add a minus sign before the column names given to .order_by() to sort in descending order and it seems to be working in the dynamic-rest sort param as well. Before, I was looking at the dynamic-rest documentation, the DRF ViewSet documentation and event the DynamicSortingFilter code here to find out if this was possible, but it didn't occur to me to look as deep as the Django QuerySet documentation. Sorry for wasting your time and thank you for nudging me in the right direction. It might be nice to document this here though. Should I update the docs and create a PR?

[aleontiev]

Ah I misunderstood -- we do support sorting by descending order, simply prefix your field with a - sign as you would with Django: for example, /clients?sort[]=-name&page=2&per_page=5.

I didn't realize that this was not documented in our README, feel free to create a PR to address this. Thanks!

[Incanus3]

I sure will, but there's one more thing I'd like to address before updating the README. Here the README says that you can sort by fields of related records as well, but it doesn't seem to work. I traced the problem to this condition which returns false for nested fields. The valid_fields_map only contains the direct serializer fields (including the DynamicRelationFields), but not the nested fields (using dot notation), so the aforementioned condition can't possibly return true for nested fields.

[aleontiev]

@Incanus3 can you give me an example? I believe that it is possible to use dot notation to sort through DynamicRelationField (e.g. sort[]=location.name should work provided that location is a DynamicRelationField)

[Incanus3]

Sure. I have two models like this:

class GastroGroup(Model):
  class Meta:
    db_table = 'gastro_groups'

  name             = CharField(max_length = 64, unique = True)
  gastro_type      = CharField(max_length = 64)
  for_register     = RequiredBooleanField()
  for_nearby_stock = RequiredBooleanField()
  for_main_stock   = RequiredBooleanField()

class Recipe(Model):
  class Meta:
    db_table = 'recipes'

  plu    = IntegerField()
  name   = CharField(max_length = 64, unique = True)
  type   = CharField(max_length = 64) # this should be an enum
  amount = DecimalField(max_digits = 12, decimal_places = 3, default = 1)

  register_gastro_group     = ForeignKey(GastroGroup, null = True, blank = True, related_name = '+')
  nearby_stock_gastro_group = ForeignKey(GastroGroup, null = True, blank = True, related_name = '+')
  main_stock_gastro_group   = ForeignKey(GastroGroup, null = True, blank = True, related_name = '+')

and their serializers:

GastroGroupSerializer():
    id = IntegerField(label='ID', read_only=True)
    name = CharField(max_length=64, validators=[<UniqueValidator(queryset=GastroGroup.objects.all())>])
    gastro_type = CharField(max_length=64)
    for_register = BooleanField()
    for_nearby_stock = BooleanField()
    for_main_stock = BooleanField()

RecipeSerializer():
    id = IntegerField(label='ID', read_only=True)
    register_gastro_group = DynamicRelationField('datastore.serializers.GastroGroupSerializer')
    nearby_stock_gastro_group = DynamicRelationField('datastore.serializers.GastroGroupSerializer')
    main_stock_gastro_group = DynamicRelationField('datastore.serializers.GastroGroupSerializer')
    plu = IntegerField(max_value=2147483647, min_value=-2147483648)
    name = CharField(max_length=64, validators=[<UniqueValidator(queryset=Recipe.objects.all())>])
    type = CharField(max_length=64)
    amount = DecimalField(decimal_places=3, max_digits=12, required=False)

Now, filtering by nested attributes, e.g. /datastore/recipes?filter{register_gastro_group.name.icontains}=test works like a charm, but when trying to sort by them, e.g. /datastore/recipes?sort[]=register_gastro_group.name, I keep getting "Invalid filter field: ['register_gastro_group.name']".

Looking at the code here, this makes sense, since valid_fields_map is populated using get_valid_fields(), which only takes direct serializer fields (i.e. serializer_class().fields) into consideration.

[aleontiev]

You are absolutely right, this implementation does not supported nested fields! It will take a bit of work, but we should be able to tweak DynamicSortingFilter to add this level of support. We may need to rethink the base implementation'sget_valid_fields because it currently operates at the view level (e.g. ordering_fields is defined there), and we would really like something at the serializer level that supports recursively validating nested ordering through related serializers.

To be honest, I'm not sure when we will be able to get to this as we have some other priorities. If you would like to take a stab at it, let me know and I'll hold off on my end.

[Incanus3]

If I understand the problem correctly, this should be a simple matter of splitting stripped_term on dots and walking through the segments, starting with serializer_class() and checking that for each (but last) segment the type(serializer_class().fields[segment]) is dynamic_rest.fields.fields.DynamicRelationField and if so, use field.serializer as the next step. Then for the last segment, we just need to check that serializer.fields[segment] is not a dynamic field. If all this is true, we can just join the segments together with '__' (double underscore) and pass it to queryset.order_by(). Is this correct or am I forgetting something?

[aleontiev]

Generally that sounds right. In this case get_valid_fields would be returning the double-underscore representation of any nested fields that ordering was requested for and get_ordering would not need to be modified, which sounds reasonable. There's also the matter of the ordering_fields parameter, which is currently used to restrict ordering choices, is defined at the view-level and does not support nested fields. There are a few options I can think of for dealing with that:

1. Allow the view to determine exactly which nested fields can be ordered by, e.g. your Recipe view might specify: ordering_fields = ('name', 'register_gastro_group.name')

2. Move ordering_fields to the serializer level, such that each serializer can control which fields can be ordered by at that level, e.g. your Recipe serializer and GastroGroup serializer might both define ordering_fields = ('name', ) in their inner Meta class, which would allow you to do sort[]=name or sort[]=register_gastro_group.name starting from the Recipe endpoint.

3. Ignore ordering_fields -- we don't have/need this for filter, but it is a Django REST Framework feature that we wanted to maintain in the first implementation of this DREST feature.

[Incanus3]

I don't think it should be necessary to build the complete list of all possible (deeply nested) ordering chains. This is working for me. You can also narrow down the list of allowed filters by setting ordering_fields = ('name', 'register_gastro_group.name') on the view level. Is this enough?

[aleontiev]

👍 that sounds great, I was not suggestion that we should generate a complete list of ordering chains (the default behavior of ordering_fields is that an empty value means that all fields can be ordered by), just want to make sure that ordering_fields still works with nested fields in a reasonable way.

[Incanus3]

Ok, my solution doesn't work when serializer field names differ from the model field names, I'll try to address that tomorrow.

[Incanus3]

I fixed the case where serializer field names differ from model field names. I made it pass through make lint and make test. I also tried to run make tox, but I'm getting a lot of errors during requirements installation (mostly python3.4). The test runs that it managed to run are all green though. Can you take a look at the solution here and especially this and this comment?

[aleontiev]

The solution looks correct to me, couple comments:

• Not sure why you are getting errors on requirements in Python3.4, which dependencies are failing to install?
• "do we need to take the filter conditions into consideration?" -- if I understand your question correctly, no; the final queryset will have filtering and ordering applied to it, in that order. Ordering should be consistent regardless of how the result is filtered
• "for some reason, both field.serializer and field.get_serializer() return instance of correct serializer class, but with no fields" -- both .serializer and .get_serializer pass in some extra kwargs into the serializer's initializer, including the request fields, which modify the result of serializer.fields. My recommendation if you want to just get all serializer fields is to use the serializer_class property of DynamicRelationField, e.g. serializer = relation.serializer_class(), this will avoid the dynamic initialization and will be faster

[Incanus3]

Hi,
ad point 2 - that's not what I meant there. In the (commented out) comprehention here taken from the original get_valid_fields method, there are two filter conditions:

if (not getattr(field, 'write_only', False) and not field.source == '*')

The question was, are these still needed? And if so, do we need to check these on every segment of the chain? Also, why do we need to use getattr to get the write_only attribute? Is it because it may or may not be present? Isn't that a bit unclean?

point 3 - solved (the links point to master version of the file, so if you refresh them, you'll see the updated version)

[Incanus3]

Um, I don't want to be obtrusive, but is this receiving any attention? I can have this finished in half an hour if you just answer my last question.

[aleontiev]

I believe the check field.source == '*' is important because it means the underlying source is unknown so there is no Django to map to. The write_only check is there to prevent sorting by fields which cannot be the response, but I think its reasonable to ignore this for the purposes of sorting.

Sorry about the delay in response

[Incanus3]

Hi, I'm so sorry, between my last question and your answer I was assigned a high priority ticket from a completely different project and had to work on it since. But the changes should now be ready for a final review. You can check the final changes here. I made it pass through make lint and make test. As I said before, I also tried to pass it through make tox, but wasn't able to install python3.3 and 3.4 due to issues with ssl 1.1, all the other version combinations are passing though.

[Incanus3]

@aleontiev could you please look at this and tell me, if there's sth else I need to do (beside rebase, squash and PR) to get this merged?

[masterfloda]

Hey guys! I just ran into the same issue and wanted to ask if there is any progress here before I try fixing it myself...

[Incanus3]

This should be ready to merge since the end of July. Just ping me when you're ready and I'll rebase and create a PR. @aleontiev

[kumaran-nc]

Hi everyone. I'd just like to ask about the status of this issue. Will @Incanus3's fix be merged in soon?

[shopro]

Also interested when this will be implemented?

[joshvillbrandt]

Also interested in this! I'll try using @Incanus3's branch directly for now. Please merge and push to PyPi though. Thanks!

[ryochiji]

@Incanus3 - Would you be willing to open a PR?

[Incanus3]

Sure, I was willing to do that from the start. But it will probably take me a few days to rebase (hopefully not much has changed in the concerned part of the codebase), since I've got a lot of other work right now. Should be ready after the weekend though.

[joshvillbrandt]

I managed to get your branch installed and working through my setup.py file:

setup(
    install_requires=[
        # 'dynamic-rest==1.7.1',
        'dynamic-rest==1.6.8',
    ],
    dependency_links=[
        # https://github.com/AltSchool/dynamic-rest/issues/171
        'https://github.com/Incanus3/dynamic-rest/archive/feature/ordering-by-related-fields.tar.gz#egg=dynamic-rest-1.6.8'
    ],
)

I noticed at least one regression in functionality I depend on since the branch is currently off of an older version of Dynamic REST, so I'm looking forward to the rebase. Thanks in advanced, @Incanus3! Hopefully we can get @aleontiev to merge it this time too!

[Incanus3]

Sorry guys, I've been assigned some high priority work and won't have time for this for at least 2 weeks.

[joshvillbrandt]

I rebased and open a merge request! Please review @Incanus3 and @aleontiev!

[joshvillbrandt]

Good news, folks! This feature is now on the #develop branch.

Thanks for the initial code, @Incanus3, and for the review and merge, @ryochiji! Looking forward to having this in a release on pypi. 😉 😁

[Incanus3]

That is great news! No problem, it wasn't that hard. I'm sorry I couldn't be of more assistance in the end. I've been quite busy these last few weeks.

[simkimsia]

Is this live on the master branch?

[simkimsia]

I think this is solved can we close this?