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

Fix bug where calling getItem without name text would cause error #2898

Merged
merged 1 commit into from
Dec 5, 2017

Conversation

karelhala
Copy link
Contributor

Fixes error when calling report data API with id would throw error on items without name

When performing call sendDataWithRx({"action": "get_item", "controller": "reportDataController", "data": ["10r1107"]}) on items without name it would throw error. This PR fixes such issue and enables to call get_item with full ID sendDataWithRx({"action": "get_item", "controller": "reportDataController", "data": [10000000001107]})

@karelhala
Copy link
Contributor Author

@miq-bot add_label bug, gaprindashvili/yes, gtls
@izapolsk this PR fixes issue you reported to me.
@miq-bot assign @martinpovolny

@himdel
Copy link
Contributor

himdel commented Dec 1, 2017

@karelhala We probably should not support numeric ids at all... this will work only until the number is high enough not to work. We can only supports strings.

@himdel
Copy link
Contributor

himdel commented Dec 1, 2017

..the only way this can work reliably is that both id and long_id are always a string, the value in the rx message is also a string, and the data in GTL must also be a string.

Otherwise, this wont work with ids in regions with large number.

So I suggest dropping the type check, and simply looking at both id and long_id, oring the result.

@karelhala
Copy link
Contributor Author

@himdel sound reasonable. Dropped the typecheck and identificator is now check for long_id and id. Although I would rather keep simple comparison == without type check so we don't have to rely on type in GTLs and in Rx's params.

@himdel
Copy link
Contributor

himdel commented Dec 1, 2017

@karelhala Agreed on ==, makes sense if we're not 100% sure a number will never come in.

(In the mean time, I talked with QE and seems like they don't use a region value that high, so it will probably work even if they do send a number.)

((That said, we may need QE to test even with larger region numbers at some point, so maybe we will still have to add a check later.))

Another thing I noticed, long_id still comes as a number from the server.. doing a PR to fix that, but not sure if that can go to gaprindashvili safely (#2902).

@himdel
Copy link
Contributor

himdel commented Dec 1, 2017

@karelhala Maybe you forgot to push? I'm still seeing that typeof identifier === "string" - https://github.com/ManageIQ/manageiq-ui-classic/pull/2898/files#diff-919045e0fab39ba4125aa7bc96d16a3eR114

@martinpovolny
Copy link
Member

(In the mean time, I talked with QE and seems like they don't use a region value that high, so it will probably work even if they do send a number.)

We should suggest them that they actually DO use a high enough number to make things fail.

pong @mfalesni, @jkrocil

@karelhala karelhala force-pushed the gtlAPIImprovements branch 2 times, most recently from 6ec80b1 to 472c608 Compare December 4, 2017 15:03
@miq-bot
Copy link
Member

miq-bot commented Dec 4, 2017

Checked commit karelhala@893cd98 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@himdel himdel added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 5, 2017
@himdel himdel merged commit 6bae823 into ManageIQ:master Dec 5, 2017
@himdel
Copy link
Contributor

himdel commented Dec 5, 2017

Merged, LGTM now :)

We should suggest them that they actually DO use a high enough number to make things fail.

Agreed, this could be useful :).

simaishi pushed a commit that referenced this pull request Dec 11, 2017
Fix bug where calling getItem without name text would cause error
(cherry picked from commit 6bae823)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 16f3fc2b2ad6a7545a13497263bab3c86658fda9
Author: Martin Hradil <himdel@seznam.cz>
Date:   Tue Dec 5 19:01:59 2017 +0000

    Merge pull request #2898 from karelhala/gtlAPIImprovements
    
    Fix bug where calling getItem without name text would cause error
    (cherry picked from commit 6bae823c3bd57c44f163cf55236c4de563564504)

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.

None yet

5 participants