bchavez / RethinkDb.Driver

:headphones: A NoSQL C#/.NET RethinkDB database driver with 100% ReQL API coverage.
http://rethinkdb.com/api/java
Other
383 stars 134 forks source link

Object's primary key and key of RunAtom() is different (Client-side generation). #147

Closed MrFoxPro closed 4 years ago

MrFoxPro commented 4 years ago

Version Information

Software Version(s)
NuGet Package 2.3.23
.NET Core? 2.2
RethinkDB Server 2.3.6-windows / 6.1.7601
Server Windows OS? 2008 R2

What is the expected behavior?

Following client-side primary key generation guide, we should use non-public constructor to implement id generation. This is best way, but Insert().RunAtom() returns a object with regenerated uuid, even if another ctor with another public ctor with JsonConstructor attribute is defined.

What is the actual behavior?

Object should be generated without calling a constructor (idk, may it should be in config)?

Any possible solutions?

no

How do you reproduce the issue?

    public class State {

        [JsonProperty("id", DefaultValueHandling = DefaultValueHandling.Ignore)]
        public string Id { get; set; }
        public string someProperty {get; set;}
        protected State() {
            this.Id = Guid.NewGuid().ToString();
        }
        [JsonConstructor]
        public State(string someProperty) {

        }
...
var state = new State();
table.Insert(state).RunAtom<State>(conn).Id != state.Id; // true

Do you have a unit test that can demonstrate the bug?

no.

Can you identify the location in the driver source code where the problem exists?

I suppose: Connection.cs, line 300-305

protected virtual async Task<T> RunQueryAtomAsync<T>(Query query, CancellationToken cancelToken)
{
            //query.
            var res = await SendQuery(query, cancelToken, awaitResponse: true).ConfigureAwait(false);
            if( res.IsAtom )
            {
                try
                {
                    if( typeof(T).IsJToken() )
                    {
                        if( res.Data[0].Type == JTokenType.Null ) return (T)(object)null;
                        var fmt = FormatOptions.FromOptArgs(query.GlobalOptions);
                        Converter.ConvertPseudoTypes(res.Data[0], fmt);
                        return (T)(object)res.Data[0]; //ugh ugly. find a better way to do this.
                        //return res.Data[0].ToObject<T>();
                    }
                    return res.Data[0].ToObject<T>(Converter.Serializer);

                }
                catch( IndexOutOfRangeException ex )
                {
                    throw new ReqlDriverError("Atom response was empty!", ex);
                }
            }
            if( res.IsError )
            {
                throw res.MakeError(query);
            }
            throw new ReqlDriverError(
                $"The query response cannot be converted to an object of T or List<T>. This run helper works with SUCCESS_ATOM results. The server response was {res.Type}. If the server response can be handled by this run method try converting to T or List<T>. Otherwise, if the server response cannot be handled by this run helper use another run helper like `.RunCursor` or `.RunResult<T>`.");
        }

If the bug is confirmed, would you be willing to submit a PR?

Yes

Sorry for my English.

MrFoxPro commented 4 years ago

I think function should return res.Data[0].ToObject<T>();, not the (T)(object)res.Data[0];

MrFoxPro commented 4 years ago

So complicated. I tried to repeat this code with boxing/unboxing, converting to jobject and back. But ids equals anyway.

MrFoxPro commented 4 years ago

Oh. Seems like i understood wrong this function... Insert method wouldn't return object. But why i get default(T)?

bchavez commented 4 years ago

Hi Dmitriy,

You need to use .RunWrite(conn) instead. Please be confident that you see a problem in the driver source code before posting a GitHub issue for help. Supposing a problem exists is not sufficient for posting issues. If you think you might need help, please use the public help Slack and Discord channels.

The code in the driver that you referenced above is correct.

The only time (T)(object)res.Data[0]; is called is if T is typeof(T).IsJToken(); and in this case, T is State not a JObject/JArray/JValue/JToken.

Because T is State the code res.Data[0].ToObject<T>(Converter.Serializer); is executed as expected.

The reason you are receiving an empty State object that looks like default(T) is that you're deserializing JSON response data with the wrong type of T.

For example, this is what you're doing:

public class ServerWriteResponse{
  public int Inserted {get;set;}
  public int Deleted {get;set;}
}

public class State{
  public string SomeProperty {get;set;}
  public string Id {get;set;}
}

The response from for the table.Insert(state) is:

{
  "inserted": 1,
  "deleted": 0
}

Effectively, when you specify .RunAtom<State>(conn) you are saying to the driver the following:

JsonConvert.Deseralize<State>("{'inserted':1,'deleted':0}").Id != state.Id

The code above does not make any sense because the JSON insert response does not match State type. This is why you get an object that looks like default(T). The JSON data does not match the type T specified.

The correct usage is as follows:

[TestFixture]
public class Issue147
{
   public class State
   {
      [JsonProperty("id", DefaultValueHandling = DefaultValueHandling.Ignore)]
      public string Id { get; set; }
      public string someProperty { get; set; }
      protected internal State()
      {
         this.Id = Guid.NewGuid().ToString();
      }

      [JsonConstructor]
      public State(string someProperty)
      {
      }
   }

   [Test]
   public void Test()
   {
      var conn = R.Connection().Connect();
      var s = new State();
      var table = R.Db("query").Table("test");
      var result = table.Insert(s).RunWrite(conn);
      result.Inserted.Should().Be(1);
   }
}

Please remember, if you don't have 100% confidence there is an issue with driver code, please ask for help in the chat channels. I will try to answer any questions you have when I have time.

Thanks, Brian Chavez