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

Experiment with a Queryable implementation that does take field names in account #2038

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

weiznich
Copy link
Member

So first the important things: I'm totally aware that code is currently not nicely structured or documented, that needs to be fixed before we think about merging this into diesel. Currently this is only a experiment to test some things out and maybe get some feedback if we really want something like this.

Why?

A commonly mistake seems to be that people assume that deriving Queryable maps field by name and not by indices. This experiment is a try to provide something that is more in line with that expectation.

How?

The idea is basically the same as that one behind frunk's labelled generics implementation, so I just link their description .
There are a few modifications to adapt that into something that could be used to implement this feature in such a way that actual loading continues to use indices internally. The mapping is done entirely at type after that.

What is missing

Probably a lot

Open Questions:

  • How do we handle boxed queries?
  • This currently incoperates only field names, not table names. So this will fail as soon as a query involve fields with the same name from multiple tables. Maybe this is fixable
  • Do we event want this thing?
  • Is it ok to depend on frunk that way and convert forth and back to hlists
  • How to incorporate this with LoadDsl?
  • How to verify that this really optimized away?

Additional points to fix if we want this:

  • Implement FromHlist for bigger Hlist's
  • Cleanup
  • Documentation
  • More tests (specifically compile tests)

@weiznich weiznich requested a review from a team April 17, 2019 22:05
@mmrath
Copy link

mmrath commented Apr 24, 2019

This is very nice. I am assuming this will fix the issue of out of order fields with same type.

@weiznich
Copy link
Member Author

weiznich commented May 6, 2019

@diesel-rs/core Any opinions about this?

@Eijebong
Copy link
Member

Eijebong commented May 6, 2019

I kinda like the idea behind it. My main problem with it is how do we handle the fact that a column is misnamed ? Also Queryable can usually lie and the same structure could be used with multiple tables that have different names (like postfixed or prefixed).

@weiznich
Copy link
Member Author

weiznich commented May 7, 2019

My main problem with it is how do we handle the fact that a column is misnamed ?

I'm not sure what you mean exactly by "a column is misnamed".

  • Do you mean the field name in the struct does not match the name of the field in the query? That results in a compile error (that does not look that nice, but contains the expect and provided name in some gabbled form).
  • Do you mean that the struct field should have a different name than the returned column? That could probably be solved by some attribute added to the custom derive.

Also Queryable can usually lie and the same structure could be used with multiple tables that have different names (like postfixed or prefixed).

I don't think removing Queryable as it exists today is a good idea. This approach should be added as an additional option in my option. Queryable has clear use cases and should continue to exist as independent option. To only thing that may change is that some documentation like the beginner guide may point to this new trait/derive first.

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

Successfully merging this pull request may close these issues.

None yet

3 participants