cjdev2 / rest-specs

A test-friendly mechanism for expressing RESTful http contracts
GNU General Public License v2.0
2 stars 5 forks source link

Support inline json request and response representations in specification #9

Open karun012 opened 10 years ago

karun012 commented 10 years ago

To avoid having to create a separate file

jking-roar commented 10 years ago

This is already supported, but unwieldy. Instead of representation-ref use representation field. A representation is a string whose contents are the body of the request of response.

davidsiefert commented 10 years ago

Sorry, what we meant to say was, support inline json type... ie, instead of:

representation: "{ \"field\": \"value\" }",

we would support:

representation: { "field": "value" },

Would you agree this is easier to write/maintain?

jking-roar commented 10 years ago

How will you distinguish between a representation which is a string containing a json object and a representation which is a string which is a string containing a json object? (since a string is a valid json representation)

On Thu, May 15, 2014 at 9:10 AM, davidsiefert notifications@github.comwrote:

Sorry, what we meant to say was, support inline json type... ie, instead of:

representation: "{ \"field\": \"value\" }",

we would support:

representation: { "field": "value" },

Would you agree this is easier to write/maintain?

— Reply to this email directly or view it on GitHubhttps://github.com/cjdev/rest-specs/issues/9#issuecomment-43230337 .

davidsiefert commented 10 years ago

Not yet seeing this as a problem--when would you need to distinguish the two? They are both inserted into the request body. If it is a string, it is put in verbatim. If it is a JSON object, it is marshalled into a string. May I please have you provide a more detailed example of where this would break down?

ratamacue commented 10 years ago

I can think of two other potential solutions here:

  1. Maybe a cleaner way to do the following: JSONAssert.assertEquals(expectedJson, response.andReturn().getResponse().getContentAsString(), false). I like JSONAssert because it calls the following two equal: {"name":"dave", "favoriteColor":"unknown"} and {"favoriteColor":"unknown", "name":"dave"}. Maybe if there was a response.getContentAsString() since this patter is so common?
  2. Support passing a Hamcrest Matcher into the rest spec so the logic about how to match can be defined outside of the REST. Rather than Rest Specs even knowing how to parse JSON/XML/YAML/etc, just make Rest Spec dependent on the generic Hamcrest Matcher and provide a nice interface for asserting with matchers.

On Thu, May 15, 2014 at 9:14 AM, jking-roar notifications@github.comwrote:

How will you distinguish between a representation which is a string containing a json object and a representation which is a string which is a string containing a json object? (since a string is a valid json representation)

On Thu, May 15, 2014 at 9:10 AM, davidsiefert notifications@github.comwrote:

Sorry, what we meant to say was, support inline json type... ie, instead of:

representation: "{ \"field\": \"value\" }",

we would support:

representation: { "field": "value" },

Would you agree this is easier to write/maintain?

— Reply to this email directly or view it on GitHub< https://github.com/cjdev/rest-specs/issues/9#issuecomment-43230337> .

  • Josh King

— Reply to this email directly or view it on GitHubhttps://github.com/cjdev/rest-specs/issues/9#issuecomment-43230958 .

davidsiefert commented 10 years ago

@ratamacue--take a look at the rest spec mockrunner project--you should not have to do JSONAssert in your test to match against things in your rest spec. Your test can be simpler using the mockrunner, and you'd be TDD'ng with your spec as the guidepost. Feedback is welcome.

ratamacue commented 10 years ago

The rest spec mockrunner project uses a string comparison to match two JSON Objects. This is OK for lists, but not for maps. With the mockrunner project, {"name":"dave", "favoriteColor":"unknown"} is not equal to {"favoriteColor":"unknown", "name":"dave"}. With JSONAssert, they are. As an example in the CJ codebase, flip lines 15 and 16 of GetMessages.response.json and then run MessageControllerTest. That test shouldn't fail with that change in my opinion. But, other people have different opinions, with JSONAssert, we have the option to choose. If we could interact with Rest Specs using matchers, we don't impose that opinion on our users.

On Thu, May 15, 2014 at 9:28 AM, davidsiefert notifications@github.comwrote:

@ratamacue--take a look at the rest spec mockrunner project--you should not have to do JSONAssert in your test to match against things in your rest spec. Your test can be simpler using the mockrunner, and you'd be TDD'ng with your spec as the guidepost. Feedback is welcome.

— Reply to this email directly or view it on GitHubhttps://github.com/cjdev/rest-specs/issues/9#issuecomment-43232743 .

davidsiefert commented 10 years ago

@ratamacue--JSON comparison was already added to the mockrunner (see https://github.com/cjdev/rest-specs/blob/master/mockrunner/src/main/java/com/cj/restspecs/mockrunner/RestSpecServletValidator.java#L184). I was not aware at the time this was put in that people would be picky about being able to assert a json object as a string. Perhaps this option could be given to the user by changing the design of the mockrunner slightly to allow a different request/response comparison implementation to be injected (ie, follow the Open-Close Principle).

ratamacue commented 10 years ago

Ooh.. I didn't notice that change. The project I was referencing must have an older version. Sweet!

On Thu, May 15, 2014 at 10:18 AM, davidsiefert notifications@github.comwrote:

@ratamacue--JSON comparison was already added to the mockrunner (see https://github.com/cjdev/rest-specs/blob/master/mockrunner/src/main/java/com/cj/restspecs/mockrunner/RestSpecServletValidator.java#L184). I was not aware at the time this was put in that people would be picky about being able to assert a json object as a string. Perhaps this option could be given to the user by changing the design of the mockrunner slightly to allow a different request/response comparison implementation to be injected (ie, follow the Open-Close Principle).

— Reply to this email directly or view it on GitHubhttps://github.com/cjdev/rest-specs/issues/9#issuecomment-43238672 .

davidsiefert commented 10 years ago

Oops, i hit the closed button by mistake! Undo! Undo! Arghhhh!!!

jking-roar commented 10 years ago

I wouldn't want the tool to eliminate the ability to specify a response body as interesting as a string that holds a json object. The inline representation is an excellent feature that I wish the tool already had. If it's made, though, I'd like an extra field like "raw representation" indicator to fork the extraction. On May 15, 2014 9:23 AM, "davidsiefert" notifications@github.com wrote:

Not yet seeing this as a problem--when would you need to distinguish the two? They are both inserted into the request body. If it is a string, it is put in verbatim. If it is a JSON object, it is marshalled into a string. May I have you provide a more detailed example of where this would break down?

— Reply to this email directly or view it on GitHubhttps://github.com/cjdev/rest-specs/issues/9#issuecomment-43232099 .

davidsiefert commented 10 years ago

@jking-roar: Would it be better if we just turned json detection off (remove code that was implemented mentioned in my response to @ratamacue), and have it only do json comparison if request/response representation is a json object in the spec (not a json object in a string)?

To elaborate completely, these are the possible scenarios...

"representation": "not a json object here"

will do String.equals comparison, and

"representation": "{ ... }"

will do String.equals, and lastly

"representation": {
...
}

will do json equivalence (so field order in objects does not matter, and neither does spacing).

jking-roar commented 10 years ago

I would prefer the decision of how to parse be based on an explicit property, rather than some logic under the hood. On May 15, 2014 1:42 PM, "davidsiefert" notifications@github.com wrote:

@jking-roar https://github.com/jking-roar: Would it be better if we just turned json detection off (remove code that was implemented mentioned in my response to @ratamacue https://github.com/ratamacue), and have it only do json comparison if request/response representation is a json object in the spec (not a json object in a string)?

To elaborate completely, these are the possible scenarios...

"representation": "not a json object here"

will do String.equals comparison, and

"representation": "{ ... }"

will do String.equals, and lastly

"representation": {...}

will do json equivalence (so field order in objects does not matter, and neither does spacing).

— Reply to this email directly or view it on GitHubhttps://github.com/cjdev/rest-specs/issues/9#issuecomment-43262149 .

ratamacue commented 10 years ago

Agreed. Such as perhaps passing in a Hamcrest Matcher and bundling a few pre-made ones. OK, I'm done beating that horse. On May 15, 2014 8:37 PM, "jking-roar" notifications@github.com wrote:

I would prefer the decision of how to parse be based on an explicit property, rather than some logic under the hood. On May 15, 2014 1:42 PM, "davidsiefert" notifications@github.com wrote:

@jking-roar https://github.com/jking-roar: Would it be better if we just turned json detection off (remove code that was implemented mentioned in my response to @ratamacue https://github.com/ratamacue), and have it only do json comparison if request/response representation is a json object in the spec (not a json object in a string)?

To elaborate completely, these are the possible scenarios...

"representation": "not a json object here"

will do String.equals comparison, and

"representation": "{ ... }"

will do String.equals, and lastly

"representation": {...}

will do json equivalence (so field order in objects does not matter, and neither does spacing).

— Reply to this email directly or view it on GitHub< https://github.com/cjdev/rest-specs/issues/9#issuecomment-43262149> .

— Reply to this email directly or view it on GitHubhttps://github.com/cjdev/rest-specs/issues/9#issuecomment-43293020 .

jking-roar commented 10 years ago

whoops!