Add note about how the body is referenced #33935

delvedor · 2018-09-21T09:44:44Z

Hi all!
In some test, such as get_source/10_basic.yml the body is referenced as an empty string instead of $body. This pr adds a note to warn the developers about this behavior.

elasticsearch/rest-api-spec/src/main/resources/rest-api-spec/test/get_source/10_basic.yml

Lines 10 to 17 in 3a5b8a7

    
           - do: 
        
               get_source: 
        
                 index: test_1 
        
                 type:  test 
        
                 id:    1 
        
           - match: { '':  { foo: bar } }

javanna · 2018-09-21T11:06:59Z

Maybe this deserves some discussion with @elastic/es-clients as there seems to be two ways to achieve the same. But if I remember correctly, $body is used mainly for cat apis, to refer to the last obtained response body as a string, while '' refers to the parsed representation of the string (parsed into a Map by the java runner for instance).

elasticmachine · 2018-09-21T12:11:24Z

Pinging @elastic/es-core-infra

nik9000 · 2018-09-21T12:11:39Z

I'm labeling this :Core/Build because that is the label we use for the test framework.

javanna · 2018-10-02T12:25:34Z

ping @elastic/es-clients anybody has opinions on this new note added to the REST spec readme file?

Mpdreamz · 2018-10-02T12:42:08Z

@javanna, +1 on the note in general but the PR doesn't mention the distinction you've raised. It's not an old style of referring to the body it's semantically different? If so the note should reflect that b

delvedor · 2018-10-02T12:47:41Z

I've added it only because it caused me some trouble during the development fo the testing suite and it wasn't easy to debug.
I don't know if the yaml files are supposed to be immutable, but if we choose to update them, we will probably break the current existing tests.

So, in the end, I see two options:

add the note to avoid undocumented behaviors (also mentioning the difference)
Update the yaml spec to adopt a unique way to reference the body.

I'm for the option 1 :)

karmi · 2018-10-02T12:51:06Z

Not sure I see the whole picture, but I think the body should be referenced in a transparent, clear way — the empty string is rather confusing, and it's the first time I've actually noticed it.

karmi · 2018-10-02T13:06:54Z

Thinking about the example a bit more, I think it's basically a slight abuse of the YAML test DLS... The test in question should probably be written as:

match: { "foo":  "bar" }

I was surprised that this actually works, and checked I don't have any special logic for handling an empty key. In a sense, this "magically works" in the Ruby runner (as an example), but I think we shouldn't use this notation, and we should prefer a more specific match.

javanna · 2018-10-02T13:48:59Z

Agreed, I think that it can be replaced in the example mentioned above, but as far as I see we use this syntax extensively to check the response of exists API, which is expected to be a boolean based on the response code (no response body is returned for HEAD requests). I am not sure that these usages are easily replaceable. I think this is another difference between $body and '', other than the one I mentioned above which is probably true only for the java runner. I think it would be good to document this, as well as looking into replacing these usages if possible as a follow-up.

karmi · 2018-10-02T13:54:11Z

(...) we use this syntax extensively to check the response of exists API, which is expected to be a boolean based on the response code (...)

I have looked at an example:

elasticsearch/rest-api-spec/src/main/resources/rest-api-spec/test/exists/10_basic.yml

Line 19 in 2ba18f5

- is_true: ''

and I think this is another case where it's probably rather confusing for anybody from the outside. In this case, expanding the test language to support something more explicit like the following would make sense?

- response: 200

(Of course, something like response.status would make more sense, but that could mean a more heavy implementation for the language runners, involve discussion about whether response.headers is a good idea to match on, etc.)

danielmitterdorfer · 2019-01-17T13:30:58Z

I think to keep the PR focussed it would be a good first step to first document the current behavior. We can always address the suggestions here in a follow-up. @delvedor can you please go through the discussion here and update your PR accordingly?

delvedor · 2019-01-18T09:29:39Z

@danielmitterdorfer sure!
@javanna I didn't understand if the current note is valid or it should be updated with:

**Note:** $body is used mainly for cat apis, to refer to the last obtained response body as a string,
while '' refers to the parsed representation of the string (parsed into a Map by the java runner for instance).

javanna · 2019-01-18T09:43:56Z

@delvedor yes I would update the note explaining the distinction that I described above between '' and $body for strongly typed languages runners (or at least for the java runner that's how it works today).

danielmitterdorfer

Thanks for iterating! I left a suggestion.

danielmitterdorfer · 2019-01-18T16:34:32Z

rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc

@@ -316,6 +316,8 @@ Supports also regular expressions with flag X for more readability (accepts whit
                   \d+    \s+  \d{2}:\d{2}:\d{2}  \s+  \d+    \s+  \n  $/
 ....

+**Note:** `$body` is used mainly for cat apis, to refer to the last obtained response body as a string, while `''` refers to the parsed representation of the string (parsed into a Map by the java runner for instance).


I think we should start with the description and then provide an example instead of the other way around. How about a slightly different wording?

`$body` is used to refer to the last obtained response body as a string, while `''` refers to the parsed representation (parsed into a Map by the Java runner for instance). Having the raw string response is for example useful when testing cat APIs.

That's fine for me :)

@delvedor can you please change this in your PR? Then I'm happy to approve. :)

Also, please don't use force-pushing as it messes up the history on the PR. Instead, squash the commits when you merge the PR eventually.

danielmitterdorfer

Thanks for iterating. LGTM

* elastic/master: (43 commits) Remove remaining occurances of "include_type_name=true" in docs (elastic#37646) SQL: Return Intervals in SQL format for CLI (elastic#37602) Publish to masters first (elastic#37673) Un-assign persistent tasks as nodes exit the cluster (elastic#37656) Fail start of non-data node if node has data (elastic#37347) Use cancel instead of timeout for aborting publications (elastic#37670) Follow stats api should return a 404 when requesting stats for a non existing index (elastic#37220) Remove deprecated FieldNamesFieldMapper.Builder#index (elastic#37305) Document that date math is locale independent Bootstrap a Zen2 cluster once quorum is discovered (elastic#37463) Upgrade to lucene-8.0.0-snapshot-83f9835. (elastic#37668) Mute failing test Fix java time formatters that round up (elastic#37604) Removes awaits fix as the fix is in. (elastic#37676) Mute failing test Ensure that max seq # is equal to the global checkpoint when creating ReadOnlyEngines (elastic#37426) Mute failing discovery disruption tests Add note about how the body is referenced (elastic#33935) Define constants for REST requests endpoints in tests (elastic#37610) Make prepare engine step of recovery source non-blocking (elastic#37573) ...

delvedor force-pushed the fix-rest-api-spec-doc-body branch from 38be28e to a1a9f63 Compare September 21, 2018 09:50

nik9000 added the :Delivery/Build Build or test infrastructure label Sep 21, 2018

Add note about how the body is referenced

a5b0282

delvedor force-pushed the fix-rest-api-spec-doc-body branch from a1a9f63 to a5b0282 Compare January 18, 2019 09:54

javanna approved these changes Jan 18, 2019

View reviewed changes

danielmitterdorfer requested changes Jan 18, 2019

View reviewed changes

Updated note

e1e454e

danielmitterdorfer approved these changes Jan 21, 2019

View reviewed changes

danielmitterdorfer added >enhancement v7.0.0 v6.7.0 labels Jan 21, 2019

delvedor merged commit 257f3ef into elastic:master Jan 22, 2019

delvedor deleted the fix-rest-api-spec-doc-body branch January 22, 2019 08:07

delvedor added a commit that referenced this pull request Jan 22, 2019

Add note about how the body is referenced (#33935)

35b3e69

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add note about how the body is referenced #33935

Add note about how the body is referenced #33935

delvedor commented Sep 21, 2018 •

edited

Loading

javanna commented Sep 21, 2018

elasticmachine commented Sep 21, 2018

nik9000 commented Sep 21, 2018

javanna commented Oct 2, 2018

Mpdreamz commented Oct 2, 2018

delvedor commented Oct 2, 2018

karmi commented Oct 2, 2018

karmi commented Oct 2, 2018

javanna commented Oct 2, 2018

karmi commented Oct 2, 2018

danielmitterdorfer commented Jan 17, 2019

delvedor commented Jan 18, 2019

javanna commented Jan 18, 2019

danielmitterdorfer left a comment

danielmitterdorfer Jan 18, 2019

delvedor Jan 21, 2019

danielmitterdorfer Jan 21, 2019

danielmitterdorfer Jan 21, 2019

danielmitterdorfer left a comment

	- do:
	get_source:
	index: test_1
	type: test
	id: 1

	- match: { '': { foo: bar } }

Add note about how the body is referenced #33935

Add note about how the body is referenced #33935

Conversation

delvedor commented Sep 21, 2018 • edited Loading

javanna commented Sep 21, 2018

elasticmachine commented Sep 21, 2018

nik9000 commented Sep 21, 2018

javanna commented Oct 2, 2018

Mpdreamz commented Oct 2, 2018

delvedor commented Oct 2, 2018

karmi commented Oct 2, 2018

karmi commented Oct 2, 2018

javanna commented Oct 2, 2018

karmi commented Oct 2, 2018

danielmitterdorfer commented Jan 17, 2019

delvedor commented Jan 18, 2019

javanna commented Jan 18, 2019

danielmitterdorfer left a comment

Choose a reason for hiding this comment

danielmitterdorfer Jan 18, 2019

Choose a reason for hiding this comment

delvedor Jan 21, 2019

Choose a reason for hiding this comment

danielmitterdorfer Jan 21, 2019

Choose a reason for hiding this comment

danielmitterdorfer Jan 21, 2019

Choose a reason for hiding this comment

danielmitterdorfer left a comment

Choose a reason for hiding this comment

delvedor commented Sep 21, 2018 •

edited

Loading