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

#70 Added the "Exists" predicate functionality #72

Merged
merged 1 commit into from
Feb 7, 2020

Conversation

kiwiprog
Copy link
Contributor

Exists predicate works like matches/equals/etc. with HttpPredicateFields. The difference as far as Mountebank is concerned, is that exists expects boolean values to indicate whether a parameter exists or not. E.g, if we apply an equals predicate on the request body as such:

{
    "id": "1234",
    "content": {
        "title": "Hello World"
    }
}

Then the request body of an http request will need an id of '1234' and content.title of 'Hello World'.

If instead we used an exists predicate, it may look like:

{
    "id": true,
    "content": {
        "title": true
    }
}

Which specifies that the request body of an http request must have id and content.title values regardless of what the values actually are.

@kiwiprog kiwiprog changed the title Added the "Exists" predicate functionality #70 Added the "Exists" predicate functionality Jan 27, 2020
@kiwiprog kiwiprog requested a review from mattherman January 27, 2020 23:33
@kiwiprog kiwiprog mentioned this pull request Jan 27, 2020
@mattherman
Copy link
Owner

Thanks! I'll try to find time to take a look at this and #73 sometime in the next day or so.

Don't worry about the failing build, I'll take care of that later and just run the tests locally for now.

@mattherman
Copy link
Owner

I'm not sure your solution will work for all of the use cases of the exists predicate. It works for RequestBody, like in the test you added, since the type of that field is object. However, imagine you wanted to create an imposter like this one in the documentation:

"predicates": [
        {
          "exists": {
            "query": {
              "q": true,
              "search": false
            },
            "headers": {
              "Accept": true,
              "X-Rate-Limit": false
            }
          }
        }
      ]

You wouldn't be able to do this with the current HttpPredicateFields class since QueryParameters and Headers are of the type IDictionary<string, string>, rather than IDictionary<string, bool>.

I had added the HttpBooleanPredicateFields a while ago to support proxies and I had hoped at the time that I could use that for the exists predicate, but I ran into a similar issue. I could only specify a boolean value for each predicate field itself, rather than their contents.

The only other option that comes to mind right now is updating QueryParameters and Headers to be of type IDictionary<string, object> which would allow you to set actual string values in addition to boolean ones. It is a possible solution if we can't think of anything better, but I don't love it since conceptually headers and query parameters shouldn't be objects.

Do you have any other ideas?

@mattherman
Copy link
Owner

I was also able to fix the build so I re-ran it on both your PRs.

@mattherman
Copy link
Owner

Thought about this a little more today and I'm fine with the IDictionary<string, object> change for Headers and QueryParameters. It will probably be the simplest solution.

That change will not be backwards-compatible though so it will need to be part of the next major version (v5) of the package that I'm currently working on. I'm not sure when 5.0.0 will be published since I have quite a few changes I'd like to add before that, but I've been publishing pre-release versions of that package for a while. I will probably just push another pre-release after this is merged.

@kiwiprog
Copy link
Contributor Author

kiwiprog commented Jan 30, 2020

Ah I see what you mean. Looks like I've not used that exists with headers/query params before with MbDotNet and overlooked it.

I believe the move to Dictionary<string, object> for headers is definitely in the right direction, especially since it is possible to have duplicate header names, the most common example being cookies I believe, see this for example and this for the Mountebank implementation.

I've experimented with this in the past and I think the correct way to specify multiple headers with the same name in mountebank is like so:

"headers": {
    "testHeader": ["123", "456"]
}

Run through Postman, I get the following in the response:
image

As such, it may need to support collections too, and Dictionary<string, object> can support it.

@kiwiprog
Copy link
Contributor Author

Just for fun's sake and throwing out ideas: if we really wanted to restrict the types of values users can set, we could write a custom BoolOrString class which can implicitly accept *cue drumroll* bool or string values. It will also need a custom JSON converter to serialize them properly.

The BoolOrString class:

[JsonConverter(typeof(BoolOrStringJsonConverter))]
public class BoolOrString
{
    private readonly object _value;

    private BoolOrString(object value)
    {
        _value = value;
    }

    public static implicit operator BoolOrString(bool input)
    {
        return new BoolOrString(input);
    }

    public static implicit operator BoolOrString(string input)
    {
        return new BoolOrString(input);
    }

    public object GetValue()
    {
        return _value;
    }
}

The custom JSON Converter:

public class BoolOrStringJsonConverter : JsonConverter
{
    public override bool CanConvert(Type objectType)
    {
        return true;
    }

    public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
    {
        var boolOrStringValue = value as BoolOrString;
        writer.WriteValue(boolOrStringValue.GetValue());
    }

    public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer)
    {
        throw new NotImplementedException();
    }
}

The usage:

var myDictionary = new Dictionary<string, BoolOrString>();

myDictionary["boolType"] = true;
myDictionary["stringType"] = "hello world";

Console.WriteLine(JsonConvert.SerializeObject(myDictionary));

Which would ultimately result in the following JSON:

{
    "boolType": true,
    "stringType": "hello world"
}

But perhaps it's slightly ugly.

@kiwiprog
Copy link
Contributor Author

Alternatively, have you considered using a different HttpPredicateFields class specific to the exists case? Sort of like HttpBooleanPredicateFields but more granular? It would leave HttpPredicateFields backwards compatible.

It's also possible to leave full control in the hands of the user and to use the object type for headers/query parameters like RequestBody, though, I get the sense that's not where you plan to go with this discussion :D

I'm still a fan of the Dictionary<string, object> approach, as it's unlikely to be used beyond that (according to my knowledge at the time of writing).

@mattherman
Copy link
Owner

I had considered both of those options as well. I really don't care for the added complexity of a custom BoolOrString type. It's too bad we don't have union types like in F#. The extra predicate fields class was slightly more appealing, but I think I would rather sacrifice some strictness in HttpPredicateFields in order to avoid adding even more types.

@mattherman
Copy link
Owner

I'm going to go ahead and merge this and just fix the other types in a subsequent commit.

@mattherman mattherman merged commit e3b3bf6 into mattherman:master Feb 7, 2020
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