Skip to content

Comments

Return true from index-exists? only on status=200#25

Open
devn wants to merge 7 commits intothreatgrid:masterfrom
devn:devn/fix-index-exists
Open

Return true from index-exists? only on status=200#25
devn wants to merge 7 commits intothreatgrid:masterfrom
devn:devn/fix-index-exists

Conversation

@devn
Copy link

@devn devn commented May 4, 2022

See #24 for additional detail.

Copy link
Contributor

@ereteog ereteog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you!
Could you add a dedicated test please?

@devn
Copy link
Author

devn commented May 5, 2022

I've added a few tests. If you'd prefer these run within the context of multiple es versions, let me know, however it did not seem necessary as the behavior for exists has been consistent from 1.5 to 8.2 (current).

Copy link
Contributor

@ereteog ereteog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@devn
Copy link
Author

devn commented May 15, 2022

Does it look good to you, though? 😅

After thinking about it a little more, the behavior here should be similar to calls that use safe-es-read, in that it throws on 401, etc.

Unfortunately, since HEAD requests don't return a body, we can't simply reuse the existing safe-es-read here.

As I see it there are a couple of options that come to mind:

  1. Create a safe-es-read-head which checks status codes and returns the code instead of body or nil, and throws on unexpected status codes as safe-es-read currently does.
  2. Use the existing safe-es-read, but make this a GET request. This would work, but violates the way you're intended to check for index existence in elasticsearch. Finally, modify index-exists? to cast the return value to boolean. This would be slightly off due to 201 in safe-es-read returning body, but it would mean this particular function could reuse safe-es-read without further modification.

Given the tradeoffs of 2, it seems better to just build a basic safe-es-read-head and update tests to check that it throws on non-200 or 404 statuses. Perhaps there's a middle option that would allow for expanding safe-es-read to work differently based on the request method, but I haven't looked closely enough to see if this would be possible.

If you have another suggestion I'd be interested to hear it, otherwise I'll go ahead and implement the option 1 when I find some time.

@devn
Copy link
Author

devn commented Jun 14, 2022

FWIW I made the change to throw on unauthorized, unknown error, etc. I think you mentioned in passing that you might have reason not to want this, but it made the most sense to me.

@ereteog
Copy link
Contributor

ereteog commented Jun 20, 2022

Yes it makes sense, thank you.
I will have a closer look later but it looks great.

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

Successfully merging this pull request may close these issues.

2 participants