Replace all characters that cannot be used as method or class name #105
Conversation
|
In my dev env I don't see any spec failure. |
|
Checked commit bzwei@6993b25 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
@bdunne do you know why the spec failed the way? |
| @@ -5,7 +5,8 @@ | |||
| "name" => "jeff", | |||
| "address" => { "street" => "22 charlotte rd"}, | |||
| "serialized_json" => { "key" => "value"}.to_json, | |||
| "serialized_yaml" => { "key" => "value"}.to_yaml | |||
| "serialized_yaml" => { "key" => "value"}.to_yaml, | |||
| ":Special$ Key3" => { "key" => "value"} | |||
There was a problem hiding this comment.
Did Tower create a new attribute with special characters in it? In the BZ and the PR description, it is not obvious to me what the problem is.
There was a problem hiding this comment.
According to the BZ, he was creating an ec2 instance with a /dev/xvda in it. When /dev/xvda being the key, the parsing and conversion code fails. I could change the test to use /dev/xvda.
There was a problem hiding this comment.
It sounds like this is buried in extra vars or something that we probably shouldn't be parsing and turning into methods, right?
There was a problem hiding this comment.
I am attempting to give a general solution to avoid coming back when another similar case arises.
Consider the following json
{
"abc" : {
"xyz": "value1",
"/dev/xvda": {
"mykey": "myvalue"
}
}
}
Ideally we should want to access value1 through abc.xyz. The second key can be accessed either by abc._dev_xvda or abc['/dev/xvda'], but its value will still be a converted model rather than raw hash.
The problem reported in BZ is not an extra var. Those we should not parse and convert are already wrapped in string (not parsable).
There was a problem hiding this comment.
I guess what I'm trying to say is that the code is currently trying to parse everything and turn it into classes and methods and we should probably only do that for the known classes.
There was a problem hiding this comment.
I am leaning with @bdunne with this. /dev/xda caught us off guard here, which means more odd cases will show up later. I think we should be white listing classes and only constantize name spaced AnsibleTowerClient classes a lot like how safe_constantize works. Depending on a regex feels brittle.
There was a problem hiding this comment.
The purpose of auto conversion in base model is to provide convenience for method style access to (nested) properties. It does NOT fundamentally change the data. You can always access the raw data treating it as a hash, ignoring any auto generated methods and classes.
Secondly the name of the auto-generated embedded class does not matter. It can be anonymous or any random valid name because what we really want is the methods. Here we just try to make the class name more meaningful.
If we want to white list classes, then we should white list methods too. Since we need to white list everything we should really abandon the base class, at least the auto conversion part. In stead we should create every class with known attributes. I am fine with this approach, but it will not to be addressed by this PR.
|
@syncrou Please review. |
| @@ -5,7 +5,8 @@ | |||
| "name" => "jeff", | |||
| "address" => { "street" => "22 charlotte rd"}, | |||
| "serialized_json" => { "key" => "value"}.to_json, | |||
| "serialized_yaml" => { "key" => "value"}.to_yaml | |||
| "serialized_yaml" => { "key" => "value"}.to_yaml, | |||
| ":Special$ Key3" => { "key" => "value"} | |||
There was a problem hiding this comment.
I am leaning with @bdunne with this. /dev/xda caught us off guard here, which means more odd cases will show up later. I think we should be white listing classes and only constantize name spaced AnsibleTowerClient classes a lot like how safe_constantize works. Depending on a regex feels brittle.
|
@bzwei were there further edit/reviews needed for this PR. |
|
@JPrause To me the fix is good for the linked BZ. The review comments were debating whether we should do auto modeling of the raw data which can be addressed in another PR. |
|
Thanks @bzwei |
|
@Fryguy Are you ok with the suggestion in #105 (comment) to address concerns in a followup PR? If so I'll open a git issue to track it. |
|
I'll defer to @bdunne On first glance this seems reasonable, but I'm concerned with we choose this and then change our minds later, then we have an API compatibility break |
|
@bdunne ping. See prior comments |
|
https://bugzilla.redhat.com/show_bug.cgi?id=1583844 becomes a blocker. ping @bdunne |
|
@bzwei I spoke with Brandon yesterday and he said he discussed this PR with you. Is there further work needed on this PR. |
|
I think I'm going to merge this for now to get the BZ out of the way. I still think we should not be creating random classes for objects that are not defined by the api, but all this PR does is prevent us from trying to create class names with invalid characters. |
|
Normally I would expect this type of change to require a major version bump, but since I see it as an unintentional "feature", I'm going with a minor version bump since it doesn't affect any of the intentionally created classes (those defined by Tower's API). |
Any key in the original hash is used in the base model for two purpose:
Either case only accepts alphabetic letters, numbers, and _. All other characters are now replaced.
fixes https://bugzilla.redhat.com/show_bug.cgi?id=1583844