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

Allow default values of Value Types to be send to docker API as query string parameters #312

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrey-skat
Copy link

For now we can't send explicitly false or 0 to docker API, because default Type values are removed from query parameters. Those values works without bugs by the fact that almost all Boolean parameters in Docker API are false by default.

But I think that creating code on the assumption that false always be default value is not good idea.
It can lead to painful debug in future. So I think we shouldn't remove those parameters from query string.

@jterry75
Copy link
Contributor

jterry75 commented May 21, 2018

This is a very common practice in the world of REST. Why send data that is the default value of its type? We have a facility to enable this by making the value optional. Consider the following:

// Go Service Side
type T struct {
    Test bool `json:",omitempty"`
}
{
    "Test": false
}

By contract this is allowing for the case where if T.Test is false what is the point of including it in json?

However, if you really want to send it from the Docker.DotNet client we can do it this way:

// C#
class T
{
    bool? Test;
}

Now its default value is null so the marshaller will skip writing it if the client has set it to any boolean. However if you set it to t.Test = false it is no longer its default value and will be forcibly marshalled even though the service assumed false by default.

Does this make sense?

@andrey-skat
Copy link
Author

You are right. But even though I'm not too familiar with the library code, I think that now this part is not obvious. Because of default Type Value there equals to default value in Docker API. But in ContainerStatsParameters Stream's default is true and parameter false is not removed just because it required.

It's not critical, but I think the reason why parameter goes to query string should be more clear. Now the reasons: it's required or it's not default type value for c# (which implicitly not default value for docker api). Instead it should be: not default value for docker api (or not passed at all, i.e. null)

I can close PR, if it is not needed :)

@jterry75 jterry75 self-assigned this May 22, 2018
@jterry75
Copy link
Contributor

Ahh. I see the issue now. Thanks for providing ContainerStatsParameters that example is perfect to identify the issue. Let me think about this for a bit and I will decide if this is the best fix of if we have other options. Leave the PR for now.

@galvesribeiro
Copy link
Member

@jterry75 have you came to a conclusion @jterry75?

Also, @andrey-skat can you please rebase it on master? Wanna do a release Today.

Thanks!

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.

3 participants