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

Linq provider does not support custom property naming conventions. #125

Closed
roald-di opened this issue Mar 8, 2018 · 1 comment
Closed

Comments

@roald-di
Copy link
Contributor

roald-di commented Mar 8, 2018

I am using snake_case_property_names in my rethinkdb tables.

While it is possible to configure the serializer to read the records in this format, it's not possible to generate queries with the linq provider that follow the same naming convention.

So for example running .Where(user => user.FirstName == "John") will result in a query with a filter containing "FirstName" instead of "first_name".

The name used in the query is determined here:

return reqlExpr[expression.Member.Name];

I think that maybe this could be extended to get the name from the ContractResolver configured on RethinkDb.Driver.Net.Converter.Serializer or the JsonPropertyAttribute if present.

However I'm not sure if introducing another dependency on Newtosoft.Json is a good idea given this issue: #33

I am willing to work on a PR if I can get some guidance on the design.

@bchavez
Copy link
Owner

bchavez commented Mar 8, 2018

Hi @roald-di,

Thank you for spending time identifying the issue with the appropriate details. I really appreciate it.

You're correct, adding another dependency on Newtonsoft.Json might not be the best way for reasons like #33 but also the added maintenance of keeping Newtonsoft.Json version numbers in sync.

As far as guidance goes:

  • I would probably suggest adding a new type to the core RethinkDb.Driver project called QueryHelper (static class). Preferably in RethinkDb.Driver\Utils\QueryHelper.cs.

  • Next, implement a static method like QueryHelper.GetJsonMemberName(...), or whatever the closest that describes the need for translating member names of type T to their serialized equivalent names in Json. Here you should be able to query Newtonsoft.Json contract for the serialized member name.

  • As far as the parameters go, I would only pass the minimal needed (lowest base type) required to get the job done. Perhaps distilling the parameters to GetJsonMemberName(Type t, string memberName), is enough. If possible, avoid passing whole Expression into the main/core driver as a parameter because I'd prefer to keep expression syntax & transversal stuff to a minimum in the core driver.

    I think it's best we keep all Linq/Expression stuff in the RethinkDb.Driver.Linq package as much as possible and only defer to the main driver (via QueryHelper) for minimal information the Linq provider needs.

  • Last but not least, write a unit test to make sure it works and that we have this issue locked down. 😄
    The unit test should probably go into RethinkDb.Driver.Linq.Tests. If you're extra thorough, another one in RethinkDb.Driver.Tests to cover QueryHelper. A good test should cover both cases snake_case and PascalCase and properties and fields (if applicable). 👍

I think this approach avoids taking a hard dependency on Newtonsoft.Json all over again in the Linq NuGet package and keeps the dependency graph as-is with the least impact.

I'm also open to other ideas, but that's how I would start. Let me know what you think.

Hope that helps,
Brian

PS. Protip: Run build restore first before opening up the VS solution.

💥 🔥 "Set it ablaze like a candle wick... Light it up, light it up..."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants