Move the OptimizedDjangoObjectType query optimization to from get_node to get_queryset

[maxpeterson]

I am looking for a way to optimise queries that are using DjangoFilterConnectionField.

Based on a quick investigation of OptimizedDjangoObjectType it looked like it ought to work if the query optimisation was moved to get_queryset, and since DjangoObjectType.get_node calls get_queryset it should remain compatible with get_node usage.

I have only looked through the code in this library superficially, so there maybe some fundamental reason that the get_queryset approach is flawed, but since the tests passed locally it seemed worth opening a PR for discussion if nothing more.

[codecov-commenter]

Codecov Report

Merging #50 into master will decrease coverage by 0.44%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
- Coverage   93.63%   93.18%   -0.45%     
==========================================
  Files           6        6              
  Lines         314      308       -6     
==========================================
- Hits          294      287       -7     
- Misses         20       21       +1     

Impacted Files	Coverage Δ
graphene_django_optimizer/types.py	100.00% <100.00%> (ø)
graphene_django_optimizer/query.py	91.82% <0.00%> (-0.39%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 08e6f59...9ea7177. Read the comment docs.

[iorlandini]

@maxpeterson maybe @tfoxy did not check this PR because of the diminution on the test coverage and because you didn't add a test for the change. But I agree with you and I created a PR only changing one line of code https://github.com/tfoxy/graphene-django-optimizer/pull/57 and adding a specific test.

[tfoxy]

@iorlandini thanks for the comment! yes, that's right. My bad that I didn't put a comment explaining that.

[maxpeterson]

Thanks @tfoxy and @iorlandini for picking this up ... and this really useful library.

It is a while ago but my impression at the time was that the change was already covered by existing tests and that the code coverage change was the result of deleting code. Anyway if #57 works with DjangoFilterConnectionField this PR is probably redundant.

[iorlandini]

Hi @maxpeterson, I think that assumption is not right. Think about if the use case was covered in the first place, we wouldn't be discussing an issue. If there is an issue, there is at least one use case not covered. And if you fix it you also have to write a test to ensure the fix will still work with further changes. Remember, tests are written for the happy and the unhappy paths.

[maxpeterson]

@iorlandini I totally get and agree that there should be a test specifically for the new / specific case calling get_queryset directly.

At the time, not being familiar with this library I was not sure if using get_queryset was an acceptable/ good approach so the PR was/is a bit of a proof of concept to see if it was an agreeable approach.

Looking closer at #57 ... I don’t think it covers my issue / case.

My case is specifically that I needed the query optimisation when get_queryset is called directly. Thus I simply moved the query optimisation call from get_node to get_queryset and removed get_node (leaving get_node in the parent to call the new get_queryset).

If you think this is a viable approach / worth pursuing then I will try and find some time to add that missing test as this is a really great library and (for me at least) would benefit from working with DjangoFilterConnectionField.

[iorlandini]

@maxpeterson I get what you are trying to say and I think you have a point. As I mentioned in my first comment, your code changes look good to me but you need to add tests and full coverage. On the other hand, I didn't make my PR based on your issue, I had one similar and I mentioned it. Maybe yours it's the one that fixes both use cases and the right call to do. I think it's worth it because I agree, this is a useful library and that's why we 3 want to make it better. If you do it, I think @tfoxy will push it forward.

[maxpeterson]

@iorlandini I realised after my first post that your PR was a similar but related issue. A test on my PR would probably have demonstrated that!

I will add test(s) for my case and and include the test to verify this approach also covers your case.

[codecov-io]

Codecov Report

Merging #50 (04bce1c) into master (bf41b97) will decrease coverage by 0.15%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
- Coverage   93.84%   93.69%   -0.16%     
==========================================
  Files           6        6              
  Lines         325      317       -8     
==========================================
- Hits          305      297       -8     
  Misses         20       20              

Impacted Files	Coverage Δ
graphene_django_optimizer/types.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bf41b97...04bce1c. Read the comment docs.

[maxpeterson]

@tfoxy and @iorlandini I added a test to assert that calling get_queryset on an OptimizedDjangoObjectType will optimise the query. Sorry I didn't get to it sooner!

I am not certain why the code coverage is failing, but I guess it is because I have removed code. That is to say there is less code now so less coverage - If this is the case it seems a bit of a failure, and I have never encountered it before but it was the only explanation I have come up with

As you can see I removed get_node, maybe_optimize and get_optimized_node from types.py. All of the is logic moved into get_queryset and since get_node calls get_queryset there is no need to to overwrite it.

If maybe_optimize and get_optimized_node are beneficial in their own right (not just to assist get_node) then they could be restored but they would require specific tests. In this respect this is probably a breaking change rather than just an enhancement.

[iorlandini]

Hi @maxpeterson, you are right, the code coverage for the project went down because you removed code, which is not a problem and that's why there is a codecov/project and a codecov/path. I think @tfoxy will agree on that.

On the other hand, code changes look good to me but I'm not sure about the test. I'll let my code review and I'd like to read @tfoxy's opinion.

[tfoxy]

BTW thanks @maxpeterson and @iorlandini for the time you are putting in this. I'm a little skeptical about merging code in this repo because it's been a long time since I used Python or Django. But you seem to know and care about this, so I'm good with merging. @iorlandini let me know if the other tests are covering the get_node method or not so we can proceed with this.

[tfoxy]

I'm merging this to move forward so we can have this feature as part of the library.

[tfoxy]

Released in v0.8.0!