An empty entry is added in undefined List<String> config item

[gsmet]

If you have a config item that is a List<String> and you don't define any value for it, you end up with a list containing one empty string element.

The expected behavior is that the list should be empty.

See #2095 (comment) and I also had the issue in the Hibernate Search extension.

[gsmet]

I think it's an issue upstream in SmallRye Config: https://github.com/smallrye/smallrye-config/blob/master/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java#L59 .

We should check if the string is empty and return an empty collection if it is.

@kenfinnigan @dmlloyd does it make sense to you?

[cristhiank]

@gsmet Actually I traced this error back to:

quarkus/core/runtime/src/main/java/io/quarkus/runtime/configuration/ConfigUtils.java

Lines 28 to 36 in a91a034

	public static <T, C extends Collection<T>> C getDefaults(SmallRyeConfig config, String defaultValue, Class<T> itemType,
	IntFunction<C> collectionFactory) {
	final String[] items = StringUtil.split(defaultValue);
	final C collection = collectionFactory.apply(items.length);
	for (String item : items) {
	collection.add(config.convert(item, itemType));
	}
	return collection;
	}

I added a fix in the Flyway PR, please let me know if it is ok there or I can open a new PR.

[gsmet]

@cristhiank let's make it a separate PR/discussion so that we can make progress on the Flyway PR independently. I think we would need to add a test in the test extension too so it's really a separate thing.

I think the issue is for the defaults and also for the values (this one is upstream). The behavior should be consistent IMHO.

I don't think your fix is right. You shouldn't filter all the empty values. I think you should just have a condition at the top testing for:

if (defaultValue == null || defaultValue.isEmpty()) {
    return collectionFactory.apply(0);
}

I wonder if this should be in the StringUtil.split() of MP config though.

@jmesnil @kenfinnigan @dmlloyd WDYT?

[cristhiank]

@cristhiank let's make it a separate PR/discussion so that we can make progress on the Flyway PR independently. I think we would need to add a test in the test extension too so it's really a separate thing.

Thanks @gsmet , I already reverted the Flyway PR to its original state

I think the issue is for the defaults and also for the values (this one is upstream). The behavior should be consistent IMHO.

Yes, you are right, the default value is "" when the config property is a list

I don't think your fix is right. You shouldn't filter all the empty values. I think you should just have a condition at the top testing for:

if (defaultValue == null || defaultValue.isEmpty()) {
    return collectionFactory.apply(0);
}

Yes, you are right, this will be way better.

I wonder if this should be in the StringUtil.split() of MP config though.

I think it shouldn't, split should do only splitting, further checks are a caller's responsibility.

@jmesnil @kenfinnigan @dmlloyd WDYT?

[kenfinnigan]

@gsmet I think the code as it stands essentially returns collectionFactory.apply(0), albeit with a few redundant calls.

I took a look at StringUtil.split(), which is in SmallRye Config, and it returns a zero length array: https://github.com/smallrye/smallrye-config/blob/master/implementation/src/main/java/io/smallrye/config/StringUtil.java#L28

If we wanted to short circuit we could check the returned items for zero length and just return

[gsmet]

@kenfinnigan in this case, the value is not null as it is defined by default = "" so it's an empty string.

If an empty string, AFAICS, you end up with a one element list, the element being the empty string.

[kenfinnigan]

So is it just a case of changing StringUtil.split() to check for empty as well?

I presume " " isn't intended to be a valid value? Or could it be?

[gsmet]

I presume " " isn't intended to be a valid value? Or could it be?

That's the big question and why I wasn't sure it could be fixed globally.

[cristhiank]

I presume " " isn't intended to be a valid value? Or could it be?

I would agree with this assumption.. I can't think on a case where someone wants an empty string as a configuration... but there will be always corner cases. I am in with including the fix in the split method as @gsmet suggested from start.

[dmlloyd]

Presently there is no case in which an empty value is allowed for a property. It's always treated as "not present", for better or worse.

[dmlloyd]

So what was the conclusion here? Will there be a separate PR to fix this in Quarkus, or will there be a SmallRye fix, or what?

[kenfinnigan]

I think it makes sense to adjust SmallRye

…

Sent from my iPhone

On May 6, 2019, at 12:26, David M. Lloyd ***@***.***> wrote: So what was the conclusion here? Will there be a separate PR to fix this in Quarkus, or will there be a SmallRye fix, or what? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[dmlloyd]

This seems strongly related to this forgotten-until-very-recently issue: smallrye/smallrye-config#84

/cc @jmesnil

[dmlloyd]

This would be fixed by smallrye/smallrye-config#102.

[cristhiank]

I see there is a new tag (release?) of smallrye that includes the fix for this.
https://github.com/smallrye/smallrye-config/releases/tag/1.3.7

Should I create I new issue to bump the smallrye version ? Or I can just do it and associate it to this issue ?
Quarkus is using 1.3.5

quarkus/bom/pom.xml

Line 47 in 6fb4b85

<smallrye-config.version>1.3.5</smallrye-config.version>

[kenfinnigan]

Associating to this issue is fine

[dmlloyd]

I see there is a new tag (release?) of smallrye that includes the fix for this.
https://github.com/smallrye/smallrye-config/releases/tag/1.3.7

Should I create I new issue to bump the smallrye version ? Or I can just do it and associate it to this issue ?
Quarkus is using 1.3.5

quarkus/bom/pom.xml

Line 47 in 6fb4b85

<smallrye-config.version>1.3.5</smallrye-config.version>

Just bumping the version isn't going to work; there are some other changes that will be necessary.

[cristhiank]

Ok, I'm building in localhost at this moment.. Will let you know

[cristhiank]

I see there is a new tag (release?) of smallrye that includes the fix for this.
https://github.com/smallrye/smallrye-config/releases/tag/1.3.7
Should I create I new issue to bump the smallrye version ? Or I can just do it and associate it to this issue ?
Quarkus is using 1.3.5

quarkus/bom/pom.xml

Line 47 in 6fb4b85

<smallrye-config.version>1.3.5</smallrye-config.version>

Just bumping the version isn't going to work; there are some other changes that will be necessary.

Yeap, looks like there were more changes in the Smallrye internals. Will try to fix them and push for your revision.

[dmlloyd]

This was fixed by #3629.