Raise a 404 exception when document source is not found (#33384) #34083

cbismuth · 2018-09-26T14:04:42Z

This pull request makes the RestGetSourceAction return a ResourceNotFoundException with a proper JSON response when source or document itself is missing (see issue #33384).

Here is below a sample JSON output:

{
  "error": {
    "root_cause": [
      {
        "type": "resource_not_found_exception",
        "reason": "Document or source not found [index1]/[_doc]/[1]"
      }
    ],
    "type": "resource_not_found_exception",
    "reason": "Document or source not found [index1]/[_doc]/[1]"
  },
  "status": 404
}

elasticmachine · 2018-09-27T08:21:19Z

Pinging @elastic/es-core-infra

…_found

cbismuth · 2018-10-10T15:49:01Z

Hi, this PR is synced with master and tests are all green, it's ready for review. I stay available for any further change, thank you.

…_found

cbismuth · 2018-10-27T14:41:01Z

Hi, this PR is synced with master and tests are all green, it's ready for review. I stay available for any further change, thank you.

nik9000

I left a couple of inline comments. One for a slightly better result and a couple of small ones about hte test. Thanks @cbismuth!

nik9000 · 2018-10-30T18:09:32Z

server/src/test/java/org/elasticsearch/rest/action/document/RestGetSourceActionTests.java

+        assertThat(restResponse.content(), equalTo(new BytesArray("{\"foo\": \"bar\"}")));
+    }
+
+    public void testRestGetSourceAction_withNullSource() {


I don't tend to like the _ in the method name. I'd just keep the camel case.

No worries, let's keep the camel case, fixed in 40a59bc.

nik9000 · 2018-10-30T18:10:20Z

server/src/test/java/org/elasticsearch/rest/action/document/RestGetSourceActionTests.java

+    }
+
+    public void testRestGetSourceAction() throws Exception {
+        // GIVEN a REST Get Source action response with an existing result and a non-null source


I don't think the GIVEN, WHEN, THEN comments buy much here.

Yes, you're right, fixed in c168b67.

nik9000 · 2018-10-30T18:10:28Z

server/src/main/java/org/elasticsearch/rest/action/document/RestGetSourceAction.java

+                final String type = response.getType();
+                final String id = response.getId();
+
+                throw new ResourceNotFoundException("Document or source not found [" + index + "]/[" + type + "]/[" + id + "]");


If you check the exists flag then is set the we could return "document not found" rather than "source not found".

Good catch! Fixed in 3fac7c9, thank you 👍

nik9000 · 2018-10-30T18:11:13Z

@elasticmachine, test this please

…_found

cbismuth · 2018-10-31T13:07:46Z

Hi @nik9000, thank you for you review, I've updated this PR and synced it with master, it's ready for review again 👍

nik9000

I left a small thing. Could you fix it for my own paranoia?

nik9000 · 2018-10-31T18:09:36Z

server/src/main/java/org/elasticsearch/rest/action/document/RestGetSourceAction.java

+         * @throws ResourceNotFoundException if source or doc itself is missing
+         */
+        private void checkIfSourceEmpty(final GetResponse response) {
+            if (response.isSourceEmpty()) {


I'd prefer to check isExists first because, at least in my head, isSourceEmpty doesn't really make sense if the document doesn't exist. I know it works to do it this way, but it'd make me feel better to switch the order.

You're totally right.

nik9000 · 2018-10-31T18:10:16Z

@elasticmachine, test this please

nik9000 · 2018-10-31T20:56:09Z

@elasticmachine, test this please

cbismuth · 2018-10-31T20:57:07Z

No paranoia at all, you're totally right, the validation workflow is better now (see c3c9be0), thank you @nik9000 👍

nik9000 · 2018-10-31T20:59:09Z

Thanks for iterating on it with me @cbismuth! I've kicked off a CI build for it. When it passes I'll merge it and backport it to 6.x. I wasn't 100% convinced that backporting to 6.x is appropriate because it is technically breaking, but empty string is already invalid JSON so I'm fine with declaring this a bug in the 6.x series and worth of this minor break.

nik9000 · 2018-10-31T21:09:43Z

@elasticmachine, retest this please

…_found

cbismuth · 2018-10-31T22:26:09Z

You're welcome @nik9000. I've just fixed Netty4HeadBodyIsEmptyIT#testGetSourceAction which failed because REST response is no longer an empty JSON (i.e. content length is now greater than 0).

Could you please trigger a CI build? Thanks you.

nik9000 · 2018-11-02T14:23:56Z

@elasticmachine, test this please

nik9000 · 2018-11-02T15:33:17Z

@elasticmachine retest this please

nik9000 · 2018-11-02T15:34:38Z

Lets give that another shot. If that doesn't pass I'd merge master in again.

cbismuth · 2018-11-02T16:05:25Z

Alright thanks @nik9000 👍

nik9000 · 2018-11-02T18:05:41Z

@cbismuth it looks like this needs master merged in because we're now depending on a different lucene snapshot. Would you like to do it?

cbismuth · 2018-11-02T19:17:29Z

Sure, I’ll be back home on Monday, I’ll do it and let you know, thank you @nik9000.

…_found

cbismuth · 2018-11-05T10:37:45Z

Hi @nik9000, PR is rebased with latest master, could you please trigger a CI build? Thanks 👍

…_found

cbismuth · 2018-11-15T13:31:42Z

Hi, PR is up-to-date with latest master and ready for merge 👍

…_found

cbismuth · 2018-11-19T10:16:27Z

PR is up-to-date with latest master and ready for build & merge, I stay available for any further change 👍

…_found

cbismuth · 2018-11-23T09:18:10Z

Hi @nik9000, I have not doubt you're hard working on many other things, so here is quick follow up:

This PR raises a 404 exception when document source is not found,
We've fixed the validation logic to check document first and then its source,
You've kindly approved this PR,
We had bad luck with bwc tests 😅

So, this PR has just been updated with master and it would be really great if we could have a build triggered again to get it merged, thanks a lot 👍

EDIT: build has started all by itself due to merge commit, I'll track this and let you know 😉

…_found

cbismuth · 2018-11-25T16:06:49Z

I've merged master into current to fix the build. Could someone please trigger another CI build? Thanks a lot 👍

nik9000 · 2018-11-26T20:56:18Z

@elasticmachine, test this please.

@cbismuth, sorry I dropped this one. I've had a rough past two weeks. I'm back now.

cbismuth · 2018-11-26T21:00:12Z

No worries @nik9000, I'm glad you're doing well 👍

cbismuth · 2018-11-27T08:32:11Z

We have a green build 👍

nik9000 · 2018-11-27T16:02:37Z

OK @cbismuth! I've merged to master and backported to 6.x and am running integration tests for that locally. I'll let you know once they pass and I've pushed to 6.x.

Thanks again!

) This pull request makes the `RestGetSourceAction` return a `ResourceNotFoundException` with a proper JSON response when source or document itself is missing (see issue #33384). Here is below a sample JSON output: ``` { "error": { "root_cause": [ { "type": "resource_not_found_exception", "reason": "Source not found [index1]/[_doc]/[1]" } ], "type": "resource_not_found_exception", "reason": "Source not found [index1]/[_doc]/[1]" }, "status": 404 } ```

cbismuth · 2018-11-28T11:42:28Z

That is really nice, thanks a lot @nik9000 for your help and support 👍

nik9000 · 2018-11-28T13:24:59Z

That backport wasn't 100% clean, but I got it! 6.x had a lot of the same warning infrastructure, but build in slightly different ways. Anyway, it is in! Thanks for sticking with this @cbismuth! It was quite a trip.

nik9000 · 2018-11-28T13:26:38Z

That backport wasn't 100% clean, but I got it! 6.x had a lot of the same warning infrastructure, but build in slightly different ways. Anyway, it is in! Thanks for sticking with this @cbismuth! It was quite a trip.

Woops, wrong issue!

Raise a 404 exception when document source is not found (#33384)

c212e7d

cbismuth mentioned this pull request Sep 26, 2018

invalid empty json on 404 INDEX/_doc/ID/_source #33384

Closed

markharwood added >bug :Data Management/Indices APIs APIs to create and manage indices and templates labels Sep 27, 2018

Merge branch 'master' into 33384_raise_resource_not_found_when_no_doc…

20ea8e3

…_found

Merge branch 'master' into 33384_raise_resource_not_found_when_no_doc…

e7f616d

…_found

nik9000 requested changes Oct 30, 2018

View reviewed changes

cbismuth added 4 commits October 31, 2018 13:49

Use camel case for test name (#33384)

40a59bc

Remove unnecessary GIVEN/WHEN/THEN test comments (#33384)

c168b67

Check missing resource type to improve exception message (#33384)

3fac7c9

Merge branch 'master' into 33384_raise_resource_not_found_when_no_doc…

04fce0b

…_found

nik9000 requested changes Oct 31, 2018

View reviewed changes

Fix validation workflow (#33384)

c3c9be0

nik9000 approved these changes Oct 31, 2018

View reviewed changes

nik9000 added the v7.0.0 label Oct 31, 2018

nik9000 added the v6.6.0 label Oct 31, 2018

cbismuth added 2 commits October 31, 2018 23:02

Merge branch 'master' into 33384_raise_resource_not_found_when_no_doc…

e514144

…_found

Fix integration test with non-empty response (#33384)

f714870

Merge branch 'master' into 33384_raise_resource_not_found_when_no_doc…

588b9a1

…_found

Merge branch 'master' into 33384_raise_resource_not_found_when_no_doc…

c3c9d94

…_found

Merge branch 'master' into 33384_raise_resource_not_found_when_no_doc…

c0a1fb4

…_found

Merge branch 'master' into 33384_raise_resource_not_found_when_no_doc…

8c2ad57

…_found

Merge branch 'master' into 33384_raise_resource_not_found_when_no_doc…

b24e989

…_found

nik9000 merged commit adc0b56 into elastic:master Nov 27, 2018

nik9000 added the backport pending label Nov 27, 2018

cbismuth deleted the 33384_raise_resource_not_found_when_no_doc_found branch November 28, 2018 11:43

nik9000 removed the backport pending label Nov 28, 2018

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

Raise a 404 exception when document source is not found (#33384) #34083

Raise a 404 exception when document source is not found (#33384) #34083

Conversation

cbismuth commented Sep 26, 2018

elasticmachine commented Sep 27, 2018

cbismuth commented Oct 10, 2018

cbismuth commented Oct 27, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Oct 30, 2018

cbismuth commented Oct 31, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Oct 31, 2018

nik9000 commented Oct 31, 2018

cbismuth commented Oct 31, 2018

nik9000 commented Oct 31, 2018

nik9000 commented Oct 31, 2018

cbismuth commented Oct 31, 2018

nik9000 commented Nov 2, 2018

nik9000 commented Nov 2, 2018

nik9000 commented Nov 2, 2018

cbismuth commented Nov 2, 2018

nik9000 commented Nov 2, 2018

cbismuth commented Nov 2, 2018

cbismuth commented Nov 5, 2018

cbismuth commented Nov 15, 2018

cbismuth commented Nov 19, 2018

cbismuth commented Nov 23, 2018 • edited Loading

cbismuth commented Nov 25, 2018

nik9000 commented Nov 26, 2018

cbismuth commented Nov 26, 2018

cbismuth commented Nov 27, 2018

nik9000 commented Nov 27, 2018

cbismuth commented Nov 28, 2018

nik9000 commented Nov 28, 2018

nik9000 commented Nov 28, 2018

cbismuth commented Nov 23, 2018 •

edited

Loading