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

Support inline json request and response representations in specification #9

Open
karun012 opened this issue May 14, 2014 · 15 comments

Comments

@karun012
Copy link

To avoid having to create a separate file

@jking-roar
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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 [email protected]:

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
.

  • Josh King

@davidsiefert
Copy link

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
Copy link
Contributor

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 [email protected]:

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 [email protected]:

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
Copy link

@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
Copy link
Contributor

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 [email protected]:

@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
Copy link

@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
Copy link
Contributor

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 [email protected]:

@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
Copy link

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

@davidsiefert davidsiefert reopened this May 15, 2014
@jking-roar
Copy link
Contributor

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" [email protected] 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
Copy link

@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
Copy link
Contributor

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" [email protected] 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
Copy link
Contributor

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" [email protected] 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" [email protected] 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
Copy link
Contributor

whoops!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants