Skip to content

Fix JSON float formatting#1635

Merged
ruflin merged 6 commits into
ruflin:masterfrom
pySilver:master
May 28, 2019
Merged

Fix JSON float formatting#1635
ruflin merged 6 commits into
ruflin:masterfrom
pySilver:master

Conversation

@pySilver
Copy link
Copy Markdown
Contributor

See #1634 for details

@ruflin
Copy link
Copy Markdown
Owner

ruflin commented May 27, 2019

Interesting addition, didn't even know this constant exists. I'm trying to think of any existing deployments we might break with this. In the case someone defined it as integer on the Elasticsearch side already and a float is sent, as far as I know it will just convert it back to integer and keep working. Any ideas on what it could break?

@ezimuel Any thoughts related to this?

@pySilver Could you also add a CHANGELOG entry and some tests to it?

@pySilver
Copy link
Copy Markdown
Contributor Author

@ruflin I can't see any way it can brake a valid setup here since on both sides data will be in a proper/expected type.

Btw, its a default JSON serialization flag in elasticsearch-php library for a long time already: elastic/elasticsearch-php#481

I'll add changelog entry in a moment, but I'm unable to add tests (I've switched back from python not so long ago and not yet familiar with PHP tests yet)

pySilver added 2 commits May 27, 2019 16:12
Adds a note about float serialization
Fix changelog link
@ezimuel
Copy link
Copy Markdown
Collaborator

ezimuel commented May 28, 2019

@ruflin, I agree to add JSON_PRESERVE_ZERO_FRACTION for json_encode, we used it in elasticserach-php, as suggested by @pySilver.
I'm not sure about the fix proposed by @pySilver, since the JSON::stringify is also used in other Elastica classes. Instead of change all the stringify calls why don't add JSON_PRESERVE_ZERO_FRACTION directly in JSON::stringify as default parameter?

@ruflin
Copy link
Copy Markdown
Owner

ruflin commented May 28, 2019

Wasn't aware we also use it already in the official client. ++ on adding it then :-)

@pySilver WDYT about the proposal from @ezimuel ?

@pySilver
Copy link
Copy Markdown
Contributor Author

@ruflin I think its totally correct and should be a default behavior for all serialization tasks.

@ruflin
Copy link
Copy Markdown
Owner

ruflin commented May 28, 2019

@pySilver Could you modify your PR to this approach?

@pySilver
Copy link
Copy Markdown
Contributor Author

@ruflin done!

@ruflin
Copy link
Copy Markdown
Owner

ruflin commented May 28, 2019

Thanks. Could you try to run make lint. For some reason the linter in CI is happy. The strange thing is the file it complains about is not touched in this PR?!?

@pySilver
Copy link
Copy Markdown
Contributor Author

@ruflin yeah, same thing on localhost. The reason is here: https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Index/Settings.php#L207 introduced 4m ago at #1569

(Bool should be bool), changing it in my pr.

Comment thread lib/Elastica/JSON.php
$args = \func_get_args();

// set defaults
isset($args[1]) ? $args[1] |= JSON_PRESERVE_ZERO_FRACTION : $args[1] = JSON_PRESERVE_ZERO_FRACTION;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Had to read this line a few times but makes sense now :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, kinda cryptic. I couldn't find easier way to append const if arg[1] is set/not set.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

$args[1] = ($args[1] ?? 0) | JSON_PRESERVE_ZERO_FRACTION;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for chiming in @chx . Should we update the code?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think so :)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@chx @pySilver Someone wants to take it? ;-)

@ruflin ruflin merged commit 63553e4 into ruflin:master May 28, 2019
@ruflin
Copy link
Copy Markdown
Owner

ruflin commented May 28, 2019

@pySilver Thanks for the fix, not sure how that Bool thingy slipped through for so long.

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.

4 participants