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

Set content-type header to application/json #7966

Merged

Conversation

ycombinator
Copy link
Contributor

Resolves #7963

@spalger spalger self-assigned this Aug 9, 2016
@ycombinator ycombinator assigned spalger and unassigned spalger Aug 9, 2016
@@ -21,6 +21,7 @@ module.exports.send = function (method, path, data, server, disable_auth_alert)
var options = {
url: '../api/console/proxy?uri=' + encodeURIComponent(path),
data: method == "GET" ? null : data,
contentType: 'application/json',
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to assign this based on the actual content type, as of now there are two content types possible, application/json for standard bodies and text/plain for newline-delimited json bodies for APIs like bulk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, good catch re: bulk. Will fix.

if (data) {
try {
JSON.parse(data);
contentType = 'application/json';
Copy link
Contributor Author

@ycombinator ycombinator Aug 9, 2016

Choose a reason for hiding this comment

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

@spalger There is (sort of) an edge case with this check. Consider the following valid _bulk API request:

POST /foo/bar/_bulk
{ "delete": { "_id": "1" } }

The content-type for this request will be set to application/json, not text/plain as it would be for multiline _bulk requests.

I'm okay with this because the body is infact valid JSON in this case. Also, Elasticsearch handles this request with its content type as application/json. Still, I wanted to point it out in case you feel differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another edge case is when users are not using the bulk API but submit slightly invalid JSON (like { name: 'val' }). Elasticsearch will accept it, but a proxy or third party would likely fail to parse it.

Ultimately though elasticsearch doesn't consider the content-type at all, which is why we didn't bother with this to begin with. The only time we should set send Content-Type: application/json is when we think the body is in fact valid JSON, so this works for me.

@ycombinator
Copy link
Contributor Author

ycombinator commented Aug 9, 2016

I am testing this PR with the following requests in Console:

# Does not set Content-Type
GET _cat/indices

# Sets Content-Type to "application/json"
POST _analyze
{
  "text": "foo bar"
}

# Sets Content-Type to "text/plain"
POST /foo/bar/_bulk
{ "index": { "_id": "1" } }
{ "foo": "bar" }
{ "delete": { "_id": "1" } }

# Edge case (sort of): Sets Content-Type to "application/json"
POST /foo/bar/_bulk
{ "delete": { "_id": "1" } }

@spalger
Copy link
Contributor

spalger commented Aug 9, 2016

LGTM

@ycombinator ycombinator merged commit af3558c into elastic:master Aug 9, 2016
@ycombinator ycombinator deleted the console/fix-content-type-header branch August 9, 2016 22:19
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
…type-header

Set content-type header to application/json

Former-commit-id: af3558c
Ikuni17 pushed a commit that referenced this pull request Aug 31, 2024
`v95.7.0` ⏩ `v95.9.0`

> [!CAUTION]
> This PR contains the final set of Emotion conversions for all EuiForm
components.
> If your plugin contains any custom CSS/styling to **EuiFormRow,
EuiFormControlLayout, EuiCheckbox, EuiRadio, or EuiSwitch**, ⚠️ make
sure to QA your UI to ensure no visual regressions have occurred! ⚠️

---

## [`v95.9.0`](https://github.com/elastic/eui/releases/v95.9.0)

- Updated `EuiSearchBar`'s optional `box.schema` prop with a new
`recognizedFields` configuration. This allows specifying the phrases
that will be parsed as field clauses
([#7960](elastic/eui#7960))
- Updated `EuiIcon` with a new `tokenSemanticText` glyph
([#7971](elastic/eui#7971))
- Added support for TypeScript 5
([#7980](elastic/eui#7980))

**Bug fixes**

- Fixed `EuiSelectableTemplateSitewide` styles when used within a
dark-themed `EuiHeader`
([#7977](elastic/eui#7977))

## [`v95.8.0`](https://github.com/elastic/eui/releases/v95.8.0)

- Updated `EuiHeaderLinks`'s mobile menu to set a slight popover padding
by default ([#7961](elastic/eui#7961))
- This can be overridden via `popoverProps.panelPaddingSize` if needed.
- Updated `EuiHeaderLink` to default to a size of `s` (down from `m`)
([#7961](elastic/eui#7961))

**Accessibility**

- Updated the `aria-label` attribute for the `EuiFieldSearch` clear
button ([#7970](elastic/eui#7970))

**Bug fixes**

- Fixed a visual bug with `<EuiDualRange showInput="inputWithPopover"
/>` form controls ([#7957](elastic/eui#7957))

**Deprecations**

- Deprecated `EuiFormRow`'s `columnCompressedSwitch` display prop. Use
`columnCompressed` instead, which will automatically account for child
`EuiSwitch`es ([#7968](elastic/eui#7968))
- Deprecated `EuiFormRow`'s `rowCompressed` display prop. Use `row`
instead for vertical forms, or `centerCompressed` for inline forms
([#7968](elastic/eui#7968))
- (Styling) Updated `EuiFormRow`'s `hasEmptySpaceLabel` prop to no
longer attempt to automatically align its content to a vertical center.
Use the `display="center"` prop for that instead
([#7968](elastic/eui#7968))

**CSS-in-JS conversions**

- Converted `EuiFormControlLayout` to Emotion
([#7954](elastic/eui#7954))
- Removed `.euiFormControlLayout--*icons` classNames and
`--eui-form-control-layout-icons-padding` CSS var. Use
`--euiFormControlRightIconsCount` or `--euiFormControlLeftIconsCount`
instead
- Converted `EuiFormLayoutDelimited` to Emotion
([#7957](elastic/eui#7957))
- Fixed `cloneElementWithCss` throwing an error when used multiple times
without a `key` prop ([#7957](elastic/eui#7957))
- Updated `cloneElementWithCss` utility to support a third argument that
allows prepending vs. appending the cloned Emotion css className
([#7957](elastic/eui#7957))
- Removed `@euiFormControlLayoutClearIcon` Sass mixin
([#7959](elastic/eui#7959))
- Converted `EuiDescribedFormGroup` to Emotion
([#7964](elastic/eui#7964))
- Converted `EuiForm`, `EuiFormHelpText`, and `EuiFormErrorText` to
Emotion ([#7966](elastic/eui#7966))
- Converted `EuiFormLabel` and `EuiFormLegend` to Emotion; Removed
`@euiFormLabel` mixin
([#7967](elastic/eui#7967))
- Converted `EuiFormRow` to Emotion
([#7968](elastic/eui#7968))
- Converted `EuiCheckbox` to Emotion
([#7969](elastic/eui#7969))
- Converted `EuiRadio` to Emotion
([#7969](elastic/eui#7969))
- Converted `EuiSwitch` to Emotion
([#7969](elastic/eui#7969))
- Removed the following Sass variables:
([#7969](elastic/eui#7969))
  - `$euiFormCustomControlDisabledIconColor`
  - `$euiFormCustomControlBorderColor`
  - `$euiRadioSize`
  - `$euiCheckBoxSize`
  - `$euiCheckboxBorderRadius`
  - `$euiSwitchHeight` (and compressed/mini variants)
  - `$euiSwitchWidth` (and compressed/mini variants)
  - `$euiSwitchThumbSize` (and compressed/mini variants)
  - `$euiSwitchIconHeight`
  - `$euiSwitchOffColor`
- Removed the following Sass mixins:
([#7969](elastic/eui#7969))
  - `euiIconBackground`
  - `euiCustomControl`
  - `euiCustomControlSelected`
  - `euiCustomControlDisabled`
  - `euiCustomControlFocused`

---------

Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants