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

Quoted string in spec value should determine string type #41

Closed
opensorceror opened this issue Dec 21, 2018 · 14 comments
Closed

Quoted string in spec value should determine string type #41

opensorceror opened this issue Dec 21, 2018 · 14 comments

Comments

@opensorceror
Copy link

opensorceror commented Dec 21, 2018

Suppose I have the following property in my typesafe-config file:

memory: "string | 50G"

the generated code using tscfg is:

memory = if(c.hasPathOrNull("memory")) c.getString("memory") else "50G"

However, if I load the config first using Typesafe's ConfigFactory.load() method, and if I then initialize the generated class using this Config object, I see this in the initialized object:

memory = "string | 50G"

Instead, I expect:

memory = "50G"

So it looks like despite generating the code correctly, initializing the generated class using a Typesafe Config object does not seem to parse the string correctly.

I highly recommend that if a key has a quoted string value in the Typesafe config file, the code generator should not implicitly convert it to bytes. Of course, if it is an unquoted string, it should convert it to bytes as is done currently.

@carueda
Copy link
Owner

carueda commented Dec 21, 2018

Suppose I have the following property in my typesafe-config file:

memory: "string | 50G"

the generated code using tscfg is:

memory = if(c.hasPathOrNull("memory")) c.getString("memory") else "50G"

Ok. And just to note that memory is of type String as indicated in the input spec.

However, if I load the config first using Typesafe's ConfigFactory.load() method, and if I then initialize the generated class using this Config object, I see this in the initialized object:

memory = "string | 50G"

Instead, I expect:

memory = "50G"

Why are you using ConfigFactory.load() to load the spec? (typesafe-config will not understand tscfg's syntax to specify types/default value/etc.) Maybe I'm just not following you here. Could you provide the concrete source spec and steps?

@opensorceror
Copy link
Author

opensorceror commented Dec 21, 2018

It seems that the generated companion object by tscfg has an apply(Config) method. This is why I'm loading the config file using ConfigFactory.load() first which gives me a Config object, and then I'm passing this Config object to the apply method of the generated case class to initialize it.

My goal is to initialize the generated case class with the provided config file, but preserving strings without them being converted to bytes. (memory: "50G" should be mapped to a memory val, with its value as String). Please let me know if I'm doing this wrong.

@carueda
Copy link
Owner

carueda commented Dec 21, 2018

So, this seems to be working as expected.

  • You have your tscfg-generated wrapper generated from the spec memory: "string | 50G", which, again, indicates that memory is of type String (that second piece 50G will just be a default value for the string)
  • Then you load a (typesafe) Config object using the concrete configuration input: memory = "string | 50G".
  • So, the captured value from that concrete configuration is, as expected, just the given string "string | 50G".

Now, perhaps what you need for your spec is to indicate (see #23 as indicated in the readme):

memory = "size | 50G"

Please provide more details (perhaps a test case if more convenient) if I'm still not understanding the issue. Thx!

@carueda
Copy link
Owner

carueda commented Dec 21, 2018

Ah, overlooked your "preserving strings without them being converted to bytes. (memory: "50G" should be mapped to a memory val, with its value as String", sorry!

In that case, both the spec memory: "string | 50G" and the config input memory: "50G" look appropriate. So, what you are saying is that what gets captured in your wrapper is not the string "50G". This would be a bug. I'll create a test case for this.

@opensorceror
Copy link
Author

I apologize if I did not explain myself very well. Yes, your last comment describes the issue accurately. But just to make sure we're on the same page and maybe to elaborate on the issue a bit:

I have an application that needs the parameter memory specified exactly as 50G. So in my typesafe-config file, I tried both the following specs:

memory: 50G
memory: "50G"

In both instances (quoted and unquoted string), tscfg converts the string to bytes. I did not expect this. Intuitively, my expectation is that if I specify an unquoted string, tscfg will convert it to bytes. However, if I specify a quoted string, it should be interpreted as a string, and not converted to bytes.

This is the core issue. To get around this, I tried specifying the type explicitly (memory: "string | 50G"), but this feels like unnecessary effort for this simple scenario. Let me know if I'm missing something obvious here.

@carueda carueda changed the title Incorrect parsing of strings Quoted string in spec value should determine string type Dec 21, 2018
@carueda
Copy link
Owner

carueda commented Dec 21, 2018

Ah, I see now. Thanks for the key info.
So, yes, good point. I just changed the title a bit to reflect your suggestion.

@carueda
Copy link
Owner

carueda commented Dec 21, 2018

Seems like Typesafe-Config's API does not offer a way to distinguish between quoted and unquoted string values for things like 50G: The library reports Quoted("50G") in both the following cases:

memory: "50G"
memory: 50G

I would expect something like Unquoted("50G") for the latter. But, in any case, this output is just the toString of the object returned by config.getValue("memory") so it would of course be completely inappropriate/unstable to base the needed new logic on such output.

So, given that there's no API for the key check that would be needed here, looks like your memory: "string | 50G" spec is the only way for your use case (and such spec is completely consistent with tscfg usage anyway).

Thoughts?

carueda added a commit that referenced this issue Dec 22, 2018
@opensorceror
Copy link
Author

Hi, was away for the past few days!

What you're saying makes perfect sense.

In that case, if I load memory: "string | 50G" using ConfigFactory.load() , and then supply this initialized Config to the apply method of the generated companion object (i.e., generated by tscfg), shouldn't tscfg recognize that this is special tscfg-specific syntax and parse it to "50G" (basically, remove string | )?

@carueda
Copy link
Owner

carueda commented Jan 3, 2019

Well, no. Loading memory: "string | 50G" through the mechanism you indicate is just going to give the string "string | 50G" for the memory key to the generated wrapper. The wrapper is not the generator and, in particular, does not understand any of the special syntax; it will simply get whatever has been provided (along with the validations as explained in the readme).

The mechanism you indicate is for loading a concrete configuration, as opposed to a configuration spec: what you give tscfg is a configuration spec (where all the special syntax takes effect for the generation of the wrapper). What you give to ConfigFactory.load() (or any other loading mechanism provided by Typesafe's Config library) is a concrete configuration (that is, with actual values for all relevant names).

If you are trying use the same configuration file for both cases (as the spec and the actual configuration instance) then note that this would be OK as long as no special tscfg syntax is used.

Hope that helps as further clarification.

@opensorceror
Copy link
Author

opensorceror commented Jan 3, 2019

I see. Yep, I see the difference now.

In that case, I'm not sure tscfg should implicitly convert the string 50G (quoted or unquoted) to bytes. Typesafe Config does not implicitly convert 50G to bytes, unless I use the getBytes method. By default, it loads it as a ConfigString$Quoted instance.

The problem with tscfg's implicit conversion is that in the generated class, a Long datatype is assigned for memory: 50G, and then even if I supply a string "50G" to the generated class, it still results in a Long with value 53687091200.

Moreover, it looks like tscfg already has a special syntax to allow the user to hint a size-in-bytes conversion. Thus, implicitly doing this does not seem warranted. Instead of having the size | hint optional, it should be mandatory if the user needs a bytes conversion.

@carueda
Copy link
Owner

carueda commented Jan 3, 2019

Again, to parse the given config spec, tscfg relies on Typesafe Config, which, unfortunately, does not provide a way to differentiate between 50G and "50G" from the given spec. Since this turns out to be a value of size type according to tscfg extraction logic (which starts by trying to most specific type possible), then that's what you get. Being able to differentiate the quoted and unquoted input would be the ideal solution (perhaps we should consider making a request in this sense to Config, although I doubt it could make it there given all the diverse formats they support, backwards compatibility, etc.).

But I think you are making a good point. Perhaps tscfg should make an exception in its extraction logic and consider string values to just be of type String even if they look like of size type. I'll look into this (as soon as time permits). Thanks again for the feedback.

@opensorceror
Copy link
Author

opensorceror commented Jan 3, 2019

I agree. Thanks for looking into this!

To make sure we're on the same page, this is my suggestion:
Size-in-bytes conversion should be explicitly (not optionally) specified by user using size | <size>. Unless this is explicitly specified, tscfg should assume it is a string by default; no implicit conversion should be done. (This is also the default behaviour of Typesafe Config.)

Is this what you're thinking?

@carueda
Copy link
Owner

carueda commented Jan 3, 2019

Yes, we're on the same page.
Hmm, so, to clean things up a bit, maybe close this issue and open a new one (say, "Size-in-bytes should be explicitly indicated in config spec") with the description you just indicated?

@opensorceror
Copy link
Author

Sure. Thanks!

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

No branches or pull requests

2 participants