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

Force quotes #382

Closed
lorenzobenvenuti opened this issue May 7, 2021 · 11 comments
Closed

Force quotes #382

lorenzobenvenuti opened this issue May 7, 2021 · 11 comments
Labels
carvel triage This issue has not yet been triaged for relevance discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution helping with an issue Debugging happening to identify the problem

Comments

@lorenzobenvenuti
Copy link

lorenzobenvenuti commented May 7, 2021

Hi,

is there any way to force quotes for string values? I understand ytt will enclose a value in quotes when the value can be parsed as a number, or it's prefixed with a "special" character (#, @, ...), for example:

#@ value = "foo"
a: #@ value
#@ value = "123"
b: #@ value
#@ value = "123.5"
c: #@ value
#@ value = "#foo"
d: #@ value

Gives

a: foo
b: "123"
c: "123.5"
d: '#foo'

I have a scenario where a value can be in the form \d+e\d+ (i.e. 74e3):

#@ value="74e3"
e: #@ value

which gives

e: 74e3

The yaml we're producing is processed by another tool that inteprets the value above as a number in scientific notation, and the value is changed (74e3 is translated into 74000.0).
Adding quotes forces the downstream system to parse the value as a string, but I didn't find a way to tell ytt to explicitly add quotes (or add some raw content to the output). Is this feature supported?

Thanks,

lorenzo

@lorenzobenvenuti lorenzobenvenuti added the carvel triage This issue has not yet been triaged for relevance label May 7, 2021
@pivotaljohn
Copy link
Contributor

Hey @lorenzobenvenuti. Thank you for the question.

It's a design goal of ytt to accept, process, and render YAML structures.

The output from ytt of YAML documents is a the Go YAML parser (v2) encoding the result which conforms to the YAML spec.

The spot where strings are encoded checks to see if the value would reliable resolve to a YAML tag and only includes quotes when it is ambiguous:
https://github.com/vmware-tanzu/carvel-ytt/blob/b4a5e57e0b7b6d2d27edd857f8c8d3907172133b/pkg/yamlmeta/internal/yaml.v2/encode.go#L319-L324

The closest value that I can think of that would encode to a quote string is to prefix or suffix the value with whitespace:

a: #@ "foo"
b: #@ "123"
c: #@ "123.5"
d: #@ "#foo"
e: #@ "74e3 "

which encodes to:

a: foo
b: "123"
c: "123.5"
d: '#foo'
e: '74e3 '

https://carvel.dev/ytt/#gist:https://gist.github.com/pivotaljohn/44e4c0f6dae722e10f1f3be769087bf3

Could that work in your situation?

@pivotaljohn pivotaljohn added helping with an issue Debugging happening to identify the problem and removed carvel triage This issue has not yet been triaged for relevance labels May 7, 2021
@lorenzobenvenuti
Copy link
Author

lorenzobenvenuti commented May 9, 2021

Hi @pivotaljohn,

thank you very much for your reply. I'll try your workaround, my initial idea was to add a trailing # to force the quotes but using spaces may be better solution since downstream systems can just trim the string.

Speaking of the YAML spec, I gave a quick look and I see two different regular expressions for floating point numbers:

  • -? [1-9] ( \. [0-9]* [1-9] )? ( e [-+] [1-9] [0-9]* )? represents the canonical form
  • -? ( 0 | [1-9] [0-9]* ) ( \. [0-9]* )? ( [eE] [-+]? [0-9]+ )? should be used for tag resolution

(I copied the regexps from the YAML spec, I guess spaces are used just to increase readability)

The one used in ytt to resolve tags is missing a ? after the second [+-], so it doesn't seem compliant with the spec (and I think that adding the ? would solve my issue).
What do you think? If you believe this is a bug I can work on a PR.

Thanks,

lorenzo

@pivotaljohn
Copy link
Contributor

@cppforlife what's the word on patching the YAML parser?

@cppforlife
Copy link
Contributor

we use https://gopkg.in/yaml.v2 (we just happen to vendor it in) so changes like these would have to go through there. i do not think it is a good idea for us to maintain our own yaml spec parsing changes on top of this library.

@lorenzobenvenuti
Copy link
Author

Hi, I didn't notice that ytt was "embedding" some code from gopkg.in/yaml.v2. Anyway I see that the latest version of gopkg.in/yaml.v2 is using a different regexp to resolve a float:

var yamlStyleFloat = regexp.MustCompile(`^[-+]?(\.[0-9]+|[0-9]+(\.[0-9]*)?)([eE][-+]?[0-9]+)?$`)
var yamlStyleFloat = regexp.MustCompile(`^[-+]?[0-9]*\.?[0-9]+([eE][-+][0-9]+)?$`)

The relevant part (at least for my issue) is [eE][-+]? vs [eE][-+].

That part was updated ~2 years ago in this commit

Thanks

@pivotaljohn
Copy link
Contributor

pivotaljohn commented Jun 7, 2021

(apologies for the delay, @lorenzobenvenuti)

Well this makes it all very interesting. 😄

... I see that the latest version of gopkg.in/yaml.v2 is using a different regexp to resolve a float ...

Okay, and so we're tacking to a conversation about upgrading to the id'ed version of the Go YAML v2 package that contains this modified regular expression.

To make progress on this idea, we'd need to:

  • determine the exact version of gopkg.in/yaml.v2 we've embedded in ytt
  • determine the exact version of gopkg.in/yaml.v2 that includes this change in the way floats are parsed
  • identify any/all breaking changes make between these versions.
    • bonus points to do the same up through v2.4

and from there we can have an informed conversation about the potential risks of making the upgrade.


FWIW: as of go-yaml/yaml@496545a (at the time of writing, the HEAD of v3), this version of the package will drop/lose comments in non-trivial situations and is not yet suitable for use in ytt.

@pivotaljohn pivotaljohn added the discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution label Jun 7, 2021
@pivotaljohn
Copy link
Contributor

For the immediate term, @lorenzobenvenuti, is one of the currently identified workarounds "sufficient" for the case?

@lorenzobenvenuti
Copy link
Author

Yes, thanks!

@pivotaljohn
Copy link
Contributor

FWIW, we have done the legwork; @lorenzobenvenuti, the commit you pointed us to was cherry-pick-able as is and we've got a PR (#483) to merge it in.

@pivotaljohn
Copy link
Contributor

Delivered in release v0.37.0.

@oxfn
Copy link

oxfn commented May 14, 2024

YAML specification supports type forcing with tags: https://yaml.org/spec/1.2.2/#24-tags

confusing_string: !!str 7e43

Result should be string like "7e43"

@github-actions github-actions bot added the carvel triage This issue has not yet been triaged for relevance label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel triage This issue has not yet been triaged for relevance discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution helping with an issue Debugging happening to identify the problem
Projects
None yet
Development

No branches or pull requests

4 participants