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

270 replacer casts #275

Merged
merged 5 commits into from Sep 3, 2021
Merged

270 replacer casts #275

merged 5 commits into from Sep 3, 2021

Conversation

cdent
Copy link
Owner

@cdent cdent commented Aug 14, 2019

This is an effort to provide the functionality described in #270 by implementing optional "casts" on the $ENVIRON and $REPLACER replacers by appending a :cast (int, str, bool, float) to the replacer name (e.g., $ENVIRON:int['FOOBAR']).

This only works when the replacer is the entirety of the message being replaced:

data:
    foo: $ENVIRON:int['FOOBAR']

not mixed in:

data:
    foo: boom $ENVIRON:int['FOOBAR'] boom

We can't do this for two reasons:

  • in part, the surrounding context may conflict with the intended value of the cast
  • but more, the way the repl function inre.sub works, they can only return strings, thus the casting has to happen after the re.sub

While the code is present to make it possible to do casts of other replacers, for the moment any cast will be ignored, mostly because it doesn't make sense for most of them.

It might, however, make sense for $COOKIE so input on that desired.

This isn't done, it needs to be confirmed as having grammatical sense, and it also needs doc update, but I'm putting it up here for some initial feedback on the format and implementation.

@cdent cdent requested a review from FND August 14, 2019 14:07
@cdent
Copy link
Owner Author

cdent commented Aug 14, 2019

@jd I suspect you might have some input on this if you can cast (ha!) your mind back to gnocchi time.

@jd
Copy link
Contributor

jd commented Aug 14, 2019

I think we mostly used ENVIRON in headers not in data, so we were cool with strings only.

Now I understand why you'd need this feature in data. I did not check the implementation, but the principle LGTM.

Copy link
Collaborator

@FND FND left a comment

Choose a reason for hiding this comment

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

The concept seems sound, though I'm not sure I can grok all the implications (also WRT the implementation).

As mentioned before, I'm a little wary because this feels like special-casing, but that's quite possibly unjustified or simply irrelevant (as you're solving an actual problem here).

if value.lower() == "true":
return True
if value.lower() == "null":
return None
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor nit:

return {
    "true": True,
    "false": False,
    "null": None
}.get(value.lower(), value)

seems more readable/idiomatic?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Perhaps, but code was already there in the previous way and changing it now would be an unrelated change.

Also that looks icky.

@cdent
Copy link
Owner Author

cdent commented Aug 25, 2019

@taget have you had a chance to try this out?

@cdent
Copy link
Owner Author

cdent commented Nov 15, 2019

@taget I'm still hoping you can verify that this solves the problems you were trying to solve

@taget
Copy link

taget commented Nov 18, 2019

@cdent
hi Chris, sorry to not response you for long time.

I tried in my test environment, works for me when cast a integer to string

yaml:

  POST: /api/cgi
  request_headers:
    content-type: application/json
  data:
    params:
      content:
        requestInfo:
          requestKey: mx_ksdionuj3djfk
          caller: vs-api
          seqId: ''
          requestId: ''
        version: '1.0'
        type: json
        requestItem:
          data:
            detail:
            - uuid: $ENVIRON['VS_UUID']
              hostIpList:
                  - $ENVIRON['VS_HOSTIP']
              migrateBandwidth: 20
              migrateTimeout: 3600
              pauseTime: $ENVIRON:str['PAUSE_TIME']
              schedulerDebug: 'on'
              compression:
                compression: mt
          method: instance_live_migrate

VS_HOSTIP="101" VS_UUID="100" PAUSE_TIME=122 gabbi-run http://127.0.0.1:8000 < a.yaml

result

{
  "params": {
    "content": {
      "type": "json",
      "requestInfo": {
        "seqId": "",
        "requestId": "",
        "caller": "vs-api",
        "requestKey": "mx_ksdionuj3djfk"
      },
      "requestItem": {
        "method": "instance_live_migrate",
        "data": {
          "detail": [
            {
              "schedulerDebug": "on",
              "migrateBandwidth": 20,
              "uuid": 100,
              "compression": {
                "compression": "mt"
              },
              "pauseTime": "122",
              "migrateTimeout": 3600,
              "hostIpList": [
                101
              ]
            }
          ]
        }
      },
      "version": "1.0"
    }
  }
}

thanks for your help :)

$ENVIRON:cast is now a workable match, but it does nothing, yet.

casting.yaml sets some basic (still failing) tests
Because of the way re.sub works with functions, we need to do
casting outside of the repl function passed to re.sub. Those
functions are only allowed to return strings, and if they
don't the results are ignored.

Therefore we manage a 'cast' attribute on the test itself
which contains the value of any 'cast' group from the the
most recent matched regex. This is then used (via APPROVED_CASTS)
to do the actual casting to the proper value.

Interesting, this work points out some problems that already
existed in any casting done with replacers: the cast is done
on the entire message, not just the replaced bit, which means
for cases like

  foo$ENVIRON:int['12']bar

the results will be not quite as expected. I'll work this out
as this series of patches progresses, but for now, consider
it a known issue.
If the replacement token is the whole of the message, then an
include :cast will be done. Otherwise there will be a RuntimeError.

This provides the precendent for the previous behavior on
$ENVIRON replacement: atoms can be cast becuase there is no
context to create a dependency problem.
@cdent cdent merged commit fcd6a76 into main Sep 3, 2021
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.

None yet

4 participants