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

Matching rules against field name with a '.' character are not working #103

Closed
GiampaLab opened this issue Jul 26, 2017 · 41 comments
Closed

Comments

@GiampaLab
Copy link

Hi,

I'm currently trying to test a contract with an Odata v4 endpoint.

As part of the Odata json format specifications the server usually includes information other than the raw data in the response called annotations.

Annotations are name/value pairs that have a dot (.) as part of the name, an example could be

"@odata.context": "http://localhost/$metadata#MyEntity/$entity"

Obviously I'm not interested in the exact value of the annotations field so I'm using type matchers and my pact looks something like:

{ "consumer": { "name": "OdataConsumer" }, "provider": { "name": "OdataProvider" }, "interactions": [ { "description": "A GET request to retrieve an Odata entity", "request": { "method": "get", "path": "/odata/MyEntity('1234')" }, "response": { "status": 200, "headers": { "Content-Type": "application/json; odata.metadata=minimal", "OData-Version": "4.0" }, "body": { "@odata.context": "http://localhost/$metadata#MyEntity/$entity", "anotherField": "someDataHere" }, "matchingRules": { "$.body.@odata.context": { "match": "type" }, "$.body.anotherField": { "match": "type" } } } } ], "metadata": { "pactSpecification": { "version": "2.0.0" } } }

As you can see the matching rule is not correctly escaping the annotation name and, as a result, it won't be executed. My understanding is that the (.) is interpreted as a path separator.

Is there any way to fix this behavior having matching rules working against fields with a (.) as part of their names?

@neilcampbell
Copy link
Member

@bethesque This is an interesting one, would to hear your thoughts.

@taliesins
Copy link

As its json we should follow the same rules. So we should be able to quote property names e.g.

$.body."@odata.context".level[2].id

this would allow existing json functionality to be leveraged. For compatibility you may choose to just allow double quotes as it probably has the widest support for json libraries, but technically both should be allowed.

@GiampaLab
Copy link
Author

GiampaLab commented Jul 27, 2017

This won't work, we don't have any control on how Pact writes matching rules so the ruby library should take care of escaping json property names with a '.'

@uglyog
Copy link
Member

uglyog commented Jul 27, 2017

The correct form is $.body["@odata.context"] (squire brackets around the quoted string). If the Ruby version is not writing the matchers in this form, could you raise an issue against that project.

@bethesque
Copy link
Member

I'll add some logic to put square brackets and quotes around names with dots in them.

@bethesque
Copy link
Member

I've raised the issue here pact-foundation/pact-support#39

@bethesque
Copy link
Member

I've updated the code and released a new version of the ruby-standalone package (1.1.1).

@neilcampbell
Copy link
Member

Will get that updated on the weekend

@GiampaLab
Copy link
Author

This is great thanks for the quick feedback and support!

@neilcampbell
Copy link
Member

Merged into master now and is available in 2.0.6-beta

@GiampaLab
Copy link
Author

GiampaLab commented Aug 23, 2017

Hi all,

I managed to test this fix only today and unfortunately is still not working.
The pact file looks fine and the match rule now is escaped correctly:

"$.body[\"@odata.context\"]": { "match": "type" }

Yet the test is failing on the producer with the following error:

Diff
--------------------------------------
Key: - is expected
+ is actual
Matching keys and values are not shown

    {
   -  "@odata.context": "http://someUrl/$metadata#myEntity/$entity"
   +  "@odata.context": "http://localhost:13259/$metadata#myEntity/$entity"
    }

   Description of differences
   --------------------------------------
   * Expected "http://someUrl/$metadata#myEntity/$entity" but got "http://localhost:13259/$metadata#myEntity/$entity" at $.@odata.context

Can you please have a look? I'm using PactNet v2.0.11-beta and PactNet-Windows v2.0.11-beta on both consumer and provider.
Sorry again for the delay.

Thanks

@neilcampbell
Copy link
Member

@GiampaLab That's interesting. Any ideas @bethesque?

@neilcampbell neilcampbell reopened this Aug 23, 2017
@bethesque
Copy link
Member

bethesque commented Aug 24, 2017

I've replicated the problem here

It's actually the @ symbol causing the problem, as well as the dot. I'll look into it.

bethesque added a commit to pact-foundation/pact-support that referenced this issue Aug 24, 2017
…h dots

JsonPath gem does not correctly look up path when the key is quoted with double quotes.
Not sure what to do if a key contains a single quote AND a dot. There is a potential bug here.

Closes: pact-foundation/pact-net#103
@bethesque
Copy link
Member

I've pushed a fix. The keys with dots needed to be quoted with single quotes, not double quotes. Can you update to 1.4.3 of the ruby standalone when you have time @neilcampbell

@neilcampbell
Copy link
Member

Awesome stuff @bethesque! Yep will do.

@neilcampbell
Copy link
Member

neilcampbell commented Aug 26, 2017

@bethesque After updating from 1.1.2 to 1.4.3, I started getting a "400 Bad Request - Invalid Hostname" coming back from my sample api when performing provider verification.

After doing some investigation it appears the windows (maybe others?) standalone is sending Host: example.org instead of Host: localhost:{port}.

I verified this by passing a custom header via --custom-provider-header "Host: localhost" to the pact-provider-verifier, and it printed INFO: Replacing header 'Host: example.org' with 'Host: localhost' to stdout when writing the verification summary.

Any ideas on this one?

@neilcampbell
Copy link
Member

@bethesque Also the new version no longer supports the following scenarios:

  1. When the body is a json string, see https://github.com/SEEK-Jobs/pact-net/blob/master/Samples/EventApi/Consumer.Tests/pacts/event_api_consumer-event_api.json#L131.
    The error returned is JSON::ParserError: 757: unexpected token at '"1.0.22"'
  2. When the body is explicitly null, see https://github.com/SEEK-Jobs/pact-net/blob/master/Samples/EventApi/Consumer.Tests/pacts/event_api_consumer-event_api.json#L144
    The error returned is JSON::GeneratorError: only generation of JSON objects or arrays allowed

Not sure at what version these things changed, but it all worked in 1.1.2

@bethesque
Copy link
Member

I've not deliberately changed anything to do with those issues, they must be side effects of something else. Perhaps the JSON version upgrade. I'll investigate.

@bethesque
Copy link
Member

"1.0.22" is not a valid json object. It needs to be either a hash or an array at the top level. There's some debate here but the new Ruby JSON library obviously takes the opinion that just a string doesn't count as a standalone serialisation.

@bethesque
Copy link
Member

Can you give me the stacktrace of the second one, as I can't reproduce it in my e2e project.

@bethesque
Copy link
Member

I've made an e2e example using the standalone packages here and have recreated the issue. Will need to give it some thought on how to fix the second example, though (and I'm willing to be convinced otherwise) I feel that the first one is a #willnotfix.

bethesque added a commit to pact-foundation/pact-ruby-standalone-e2e-example that referenced this issue Aug 27, 2017
@bethesque
Copy link
Member

Actually, no, sorry, I can't reproduce the second issue.
Commit here: pact-foundation/pact-ruby-standalone-e2e-example@823abfe
Build passing here: https://travis-ci.org/pact-foundation/pact-ruby-standalone-e2e-example/builds/268791886

Can you have a look here and see if you can work out what's different between the example and the .net code?

bethesque added a commit to pact-foundation/pact-provider-verifier that referenced this issue Aug 27, 2017
@bethesque
Copy link
Member

bethesque commented Aug 27, 2017

I've fixed the host problem in 1.4.4.

@neilcampbell
Copy link
Member

@bethesque Awesome stuff! I was on the fence about the first one as well, but yeah technically it is supported in the json spec. Happy to mark that as a #willnotfix.

For the second item, I will look into it a bit deeper and see if I can reproduce/fix.

@neilcampbell
Copy link
Member

@bethesque I recreated the problem using pact-ruby-standalone-e2e-example on OSX. In your example in pact-foundation/pact-ruby-standalone-e2e-example@823abfe you set the response body to null, it's when the request body is null where the issues arises.

@bethesque
Copy link
Member

Ah! Thanks. How strange. I'll have a look into it.

@bethesque
Copy link
Member

Now I'm playing with it, I'm unsure what a null body means. Is it the JSON string "null". Is it an empty string? Does it mean 0 Content-Length? What is our use case? I'm confused now...

@uglyog
Copy link
Member

uglyog commented Aug 27, 2017

A null body has to be a body that is the JSON null value (i.e. represented string value of 'null').

@neilcampbell
Copy link
Member

neilcampbell commented Aug 27, 2017

To me it's similar to the null use case for a response. We are explicitly saying that the value should be set to null (no body), and not be just any body (which is denoted when you omit the body key).

@bethesque
Copy link
Member

No body would be an empty string (Content-Length: 0). The only way this makes sense as a separate use case is if it's the string "null" (which is again not a valid JSON "object"). Ron, does JVM pact support a 'property' (ie. top level string, boolean, number, null)?

@neilcampbell
Copy link
Member

I am assuming if top level strings aren't supported at the moment, then empty string is not supported either?

@bethesque
Copy link
Member

Sorry, I used ambiguous terminology. Thinking of the actual HTTP request, an empty body is where Content-Length: 0. The way this would be represented to the HTTP server would probably differ by language and implementation. Some frameworks might say the body was null, and some might return an empty string.

A JSON String or empty JSON String would haveContent-Length > 0 in that it would be "" or "foo", and both would equally not be technically JSON "objects".

@neilcampbell
Copy link
Member

Yep, got you. So how should we be representing no body in a language agnostic way? My understanding was setting body: null, but maybe it's better to not set body and add a Content-Length: 0 header?

@uglyog
Copy link
Member

uglyog commented Aug 28, 2017

In Pact JVM there are the following values for JSON bodies:

  • Empty body (the Content-Length header will have a value of 0)
  • Empty string body (represented by the empty JSON string "", Content-Length will probably = 2)
  • Null body (represented by the JSON null value, Content-Length will probably = 4)
  • JSON body, which could be a number, true, false, a string, an array or an object.

In the Pact file, we represent no body and empty body differently:

Empty string bodies are a difficult one. The only way to represent it correctly is with escaped strings, e.g. "\"\"".

@neilcampbell
Copy link
Member

I have removed the null body from the consumer sample now. Will get a new release out tonight with the latest pact standalone.

@bethesque
Copy link
Member

Given we're going to be moving to the Rust impl in the future, I'm reluctant to spend a heap of time on this null body thing until someone actually has a use case for it. Can we raise an issue to document the problem, and then wait until somebody jumps up and down about it?

@neilcampbell
Copy link
Member

Sounds good to me. On which project should I document the problem?

@bethesque
Copy link
Member

https://github.com/pact-foundation/pact-mock_service, that's where the pact is written in the ruby impl.

@neilcampbell
Copy link
Member

@GiampaLab This issue has been fixed and released in 2.0.12-beta

@GiampaLab
Copy link
Author

Thank @neilcampbell, @bethesque , @uglyog I just tested the new beta release and it's working correctly, well done!

Also we are working on a library which makes producer tests trivial to implement and supports branching you can find it here

Let me know what you think!

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

No branches or pull requests

5 participants