Skip to content

Fix CouchViewOptions #125

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

Merged
merged 3 commits into from
Mar 17, 2021
Merged

Fix CouchViewOptions #125

merged 3 commits into from
Mar 17, 2021

Conversation

panoukos41
Copy link
Contributor

Hey as I was using it across a project and running some tests I came across some errors when using the CouchViewOptions.

So I changed all values to have a null default value and also be Ignored by the serializer.
This fixes a bad request error for including values as null that don't support it on the server like the following

  • group_level
  • keys
  • limit

Changed stable to use a class like update since I discovered it says that it is a boolean but it needs to be in a string form.

I used a view and a client like Postman and C# to test again all values and these were the only problem.

I believe leaving all values null and as part not included until a user sets them explicitly is better since it only sends the settings the user chooses to, the DB uses its defaults and it might even decrease the payload sent to the DB if someone cares that much about it.

I also tried sending empty options and It works as expected.

Sorry for taking so long to discover these I was busy with some other projects and didn't get to use the Implementation as soon as I wanted.

Changed so all values start as `null` and Ignored.
This also fixes a bad request error for including values as null that don't support it on the server like the following
- `group_level`
- `keys`
- `limit`

Changed `stable` to use a class like `update` since I discovered it says that it is a boolean but it needs to be in a string form
@matteobortolazzo
Copy link
Owner

Hi, thanks, I will give it a look because I think I tried to handle that that, DefaultValue should be there to skip the serialization, but maybe I made a mistake

@panoukos41
Copy link
Contributor Author

Okay, yea I did a WriteLine of the serialized options (including nullvaluehandling in options didn't help) and it would print all the values with null if they were unset and as I was poking around then I discovered the stable bits too :)

I don't how I missed it at the first and second PR sorry :/

@matteobortolazzo
Copy link
Owner

Sorry, I don't understand. Issues are still there? Because in here I test that the body contains only what's neeed

@panoukos41
Copy link
Contributor Author

Okay I did more digging and the problem only occurs when I configure my client serializer like this

.ConfigureFlurlClient(s =>
{
    s.JsonSerializer = new NewtonsoftJsonSerializer(
        new JsonSerializerSettings().ConfigureForNodaTime(DateTimeZoneProviders.Tzdb));
})

So when i change the test to use the following client

_client = new CouchClient("http://localhost", o => 
    o.ConfigureFlurlClient(s=>
    {
        s.JsonSerializer = new NewtonsoftJsonSerializer(new Newtonsoft.Json.JsonSerializerSettings());
    }));

The test fails but only when WithRequestBody is used.

Also if you try to serialize the options with JSON.NET then it will include many properties even if they were not set without using the client etc.

So the problem surfaces if you need to configure Flurl serializer as it seems.

@matteobortolazzo
Copy link
Owner

I see... Makes sense... I will find a solution

@panoukos41
Copy link
Contributor Author

Okay so setting DefaultValueHandling = DefaultValueHandling.Ignore fixed it,

Maybe add a note that this setting must be set if someone overrides the serializer? And gradually change settings that are sent on the server to have null as default and the exact setting about null values?

Or you can add explicitly the DefaultValueHandling on each property to make sure it's not set ignoring user settings?

So maybe I should make a new PR only for the stable? It won't accept it without quotes " around it.

@matteobortolazzo
Copy link
Owner

@panoukos41 I think your solution makes sense otherwise I have to override the contract resolver of a custom JsonResializer. Can you remove the DefaultValue attribute in both the class and in CouchContractResolver please? I also noticed a potential bug. In CouchContractResolver, if someone change the PropertyCaseType it changes the property name of "Couch" objects too. I'll create a separate issue for that

@panoukos41
Copy link
Contributor Author

Of course, so I will leave every option nullable and remove [DefaultValue] while I leave the option NullValueHandling = NullValueHandling.Ignore as is.

For the resolver you want me to remove the defaultValueAttribute at CouchResolver Line 28?

I could also do this for other option classes in the future if you want 😄

@matteobortolazzo
Copy link
Owner

Exactly like you said.

About removing it for other classes, thanks 😅 but they are using it in the query param so they are not parsed by JSON.NET

@panoukos41
Copy link
Contributor Author

Oh I see that's great then we created the only case when implementing views xD

I am uploading the changes I hope I did the contract resolver correct :P

@matteobortolazzo
Copy link
Owner

Nice work!!

@matteobortolazzo matteobortolazzo merged commit a78aef2 into matteobortolazzo:dev Mar 17, 2021
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.

2 participants