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

PostInvertedIndexBody Serialization #470

Closed
kealist opened this issue Jan 27, 2023 · 13 comments · Fixed by #472
Closed

PostInvertedIndexBody Serialization #470

kealist opened this issue Jan 27, 2023 · 13 comments · Fixed by #472

Comments

@kealist
Copy link
Contributor

kealist commented Jan 27, 2023

Just documenting though I think this has been fixed in master, but not a release. There are serialization errors when running the following code. I assume this was something from PostInvertedIndexBody overloading Fields property.


Message: 
ArangoDBNetStandard.Serialization.SerializationException : An error occured while Deserializing the data response from Arango. See InnerException for more details.
---- Newtonsoft.Json.JsonReaderException : Unexpected character encountered while parsing value: {. Path 'fields', line 1, position 292.

  Stack Trace: 
ApiClientBase.DeserializeJsonFromStream[T](Stream stream)
IndexApiClient.PostIndexAsync(PostIndexQuery query, PostIndexBody body, CancellationToken token)
IndexApiClient.PostInvertedIndexAsync(PostIndexQuery query, PostInvertedIndexBody body, CancellationToken token)

var collection = new PostIndexQuery { CollectionName = "document" };
var postBody = new PostInvertedIndexBody
{
    Name = "inverted",
    Fields = new List<InvertedIndexField> { new InvertedIndexField { Name = "description", Analyzer = "text_en" } }
};
await db.Index.PostInvertedIndexAsync(collection, postBody);

This has just been a hassle with the index changes in 3.10 and trying to follow the index guide, which don't work with this client.

@DiscoPYF
Copy link
Collaborator

DiscoPYF commented Feb 6, 2023

Thanks for reporting this issue @kealist

Just documenting though I think this has been fixed in master, but not a release.

You're right, it was fixed by #443 which is in master but hasn't been released yet.

Your idea of releasing pre-release packages (or even daily builds packages) is a good one: #468

@tjoubert I noticed you started merging some of the breaking change PRs into master for the next major release. We could create a pre-release package, e.g. 2.0.0-alpha.1, that will contain the fix.

As per https://learn.microsoft.com/en-us/nuget/create-packages/prerelease-packages#semantic-versioning

-alpha: Alpha release, typically used for work-in-progress and experimentation
-beta: Beta release, typically one that is feature complete for the next planned release, but may contain known bugs.
-rc: Release candidate, typically a release that's potentially final (stable) unless significant bugs emerge.

The steps would be:

  1. Create a commit and PR to update the csproj version to 2.0.0
  2. Create a pre-release in github which will tag the pre-release commit
  3. Build a nuget package from that commit

I'm happy to do that. Not sure we need step 2 though, but probably good for clarity?

@kealist Would it be useful for you?

@kealist
Copy link
Contributor Author

kealist commented Feb 6, 2023

@DiscoPYF Yes anything would be great in the short term for me so I don't have to work around things. I don't know the ideal way to do that with how releases/prereleases are done on Github, I've only done it internally at my company using gitlab CI + gitversion.

@DiscoPYF
Copy link
Collaborator

DiscoPYF commented Feb 6, 2023

Yes anything would be great in the short term for me so I don't have to work around things.

Ok, I'll confirm with @tjoubert before I create the pre-release.

Alternatively, you can build a pre-release package yourself, but I don't know if your company policy/infra allows.
Steps:

That may be a good compromise while we get something to you.

@DiscoPYF
Copy link
Collaborator

DiscoPYF commented Feb 6, 2023

@tjoubert There is also the idea of releasing this fix (#443) into a patch release? e.g. 1.3.1

While the fix contains a breaking change, it fixes an endpoint that's broken anyway with ArangoDB 3.10 so that may be acceptable. Not sure if it's a good practice, as it will be a breaking change for anyone using the feature with ArangoDB 3.9 .

@DiscoPYF
Copy link
Collaborator

DiscoPYF commented Feb 6, 2023

We could also simply create multiple major releases in a row, instead of a pre-release.

For example, instead of releasing 2.0.0-alpha.1 and later 2.0.0, release 2.0.0 with what's on master now and later release a 3.0.0 with the additional breaking changes scheduled.

That's probably the simplest right now 🤔

@tjoubert
Copy link
Contributor

tjoubert commented Feb 6, 2023

Hi @DiscoPYF. Yes, I think a 2.0.0 now and a 3.0.0 later would be the simplest. See if there is any of the other PRs that you can quickly review, approve and merge for v2.0.0. Thanks!

@DiscoPYF
Copy link
Collaborator

DiscoPYF commented Feb 7, 2023

Thanks for confirming @tjoubert . I've merged what I could for now. I will have more time next week to review the remaining pull requests.

Merged:

#446
#438
#458

I will prepare the 2.0.0 release.

@DiscoPYF
Copy link
Collaborator

DiscoPYF commented Feb 7, 2023

I've just realized that it would have been best to keep the amount of changes in 2.0.0 to the minimum, so that people affected by the bug can update more seemlessly. 🤔

@DiscoPYF
Copy link
Collaborator

DiscoPYF commented Feb 7, 2023

@kealist The release is out: https://github.com/ArangoDB-Community/arangodb-net-standard/releases/tag/2.0.0

Let us know how the update goes and whether you are able to call PostInvertedIndexAsync without issues.

@kealist
Copy link
Contributor Author

kealist commented Feb 7, 2023

@DiscoPYF It's currently throwing an exception on deserializing the response (JSON integer 5368709120 too large for int32), but I'll try to get more info

  Message: 
ArangoDBNetStandard.Serialization.SerializationException : An error occured while Deserializing the data response from Arango. See InnerException for more details.
---- Newtonsoft.Json.JsonReaderException : JSON integer 5368709120 is too large or small for an Int32. Path 'consolidationPolicy.segmentsBytesMax', line 1, position 201.

  Stack Trace: 
ApiClientBase.DeserializeJsonFromStreamAsync[T](Stream stream)
IndexApiClient.PostInvertedIndexAsync(PostIndexQuery query, PostInvertedIndexBody body, CancellationToken token)
ArangoDbGraphGenerator.CreateGraphSetupAsync() line 362
ArangoDbGraphGenerator.InitializeOntologyGraphAsync() line 88
ArangoDbGraphTests.ArangoDatabaseAndGraphGenerationTests() line 57
--- End of stack trace from previous location ---
----- Inner Stack Trace -----
JsonTextReader.ParseReadNumber(ReadType readType, Char firstChar, Int32 initialPosition)
JsonTextReader.ParseNumber(ReadType readType)
JsonTextReader.ReadNumberValue(ReadType readType)
JsonTextReader.ReadAsInt32()
JsonReader.ReadForType(JsonContract contract, Boolean hasConverter)
JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
JsonSerializerInternalReader.SetPropertyValue(JsonProperty property, JsonConverter propertyConverter, JsonContainerContract containerContract, JsonProperty containerProperty, JsonReader reader, Object target)
JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
JsonSerializer.Deserialize(JsonReader reader, Type objectType)
JsonSerializer.Deserialize[T](JsonReader reader)
JsonNetApiClientSerialization.DeserializeFromStream[T](Stream stream)
JsonNetApiClientSerialization.DeserializeFromStreamAsync[T](Stream stream)
ApiClientBase.DeserializeJsonFromStreamAsync[T](Stream stream)

@kealist
Copy link
Contributor Author

kealist commented Feb 14, 2023

@DiscoPYF Just wanted to follow up to see if there was anything that could be done in the near term to resolve this.

@DiscoPYF
Copy link
Collaborator

DiscoPYF commented Mar 7, 2023

Thanks for your contibution @kealist

Progress was a bit slow here last month. I have just created a patch release which I hope will help: https://github.com/ArangoDB-Community/arangodb-net-standard/releases/tag/2.0.1

@kealist
Copy link
Contributor Author

kealist commented Mar 8, 2023

@DiscoPYF Yep, tested on my end and everything is working fine now. Thank you!

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 a pull request may close this issue.

3 participants