Closed roald-di closed 6 years ago
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. :smile:
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). :+1:
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.
:boom: :fire: "Set it ablaze like a candle wick... Light it up, light it up..."
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: https://github.com/bchavez/RethinkDb.Driver/blob/059c7e9aedf0fb81d480ec5a3594c7ab29d22611/Source/RethinkDb.Driver.Linq/MemberNameResolver.cs#L28 I think that maybe this could be extended to get the name from the ContractResolver configured on
RethinkDb.Driver.Net.Converter.Serializer
or theJsonPropertyAttribute
if present.However I'm not sure if introducing another dependency on Newtosoft.Json is a good idea given this issue: https://github.com/bchavez/RethinkDb.Driver/issues/33
I am willing to work on a PR if I can get some guidance on the design.