gradle
Do ListProperty, SetProperty and MapProperty differentiate between "EMPTY" and "UNSET" ?
Follow up on (https://github.com/gradle/gradle/issues/7485#issuecomment-585289792)
I need to be able to distinguish between EMPTY and UNSET MapProperty because a user might override a previously set property with an empty one. I'm doing mapProperty.set(null as Map<String, String>?) to unset the value from the initial empty value.
But somehow it is possible to .put() values into this empty MapProperty:
Expected Behavior
mapProperty.set(null as Map<String, String>?)
mapProperty.put("key", "value")
mapProperty.isPresent // this should be true
Current Behavior
mapProperty.set(null as Map<String, String>?)
mapProperty.put("key", "value")
mapProperty.isPresent // this is currently false
Context
See https://github.com/apollographql/apollo-android/issues/1987 for some context. The user is able to add to the MapProperty but their configuration is not picked up.
I contributed the original MapProperty in 5.1, so I'll try to explain the concepts as I understand them. Some additions have been made since then, but I believe the fundamentals are still true.
Collection and map properties use the concept of "collectors" to allow for lazy evaluation. Each collector can contribute a number of elements to the collection or entries to the map (possibly zero). When the property is evaluated, it iterates through all of the registered collectors to determine the actual value.
All such properties are initialized with a single collector that just contributes an empty collection or map. This wasn't always the case, but makes them much more usable and predictable, because starting with an empty collection/map is what you'd expect in most cases. It is also one of the big differences to the "simple" properties which start without a value.
When adding to a collection property or putting into a map property, you're really just registering another collector that will provide the given values when it is evaluated. If the collection/map starts empty, this leads to what you would expect. When you set the property to an explicit value, you discard all the existing collectors and replace them with one providing the given value.
However, when explicitly un-setting the property (by setting it to null), you replace the initial "empty" collector with a special collector that says "no value at all", and will short-circuit any evaluation of the property to "no value". It is still possible to add further collectors (hence the call to put is allowed) because of the lazy evaluation, but they will have no effect.
I agree that this behavior is quite confusing, but I think it's the only way to uphold the general Provider/Property contract while still having a decent solution for working with collections and maps. I would suggest to avoid un-setting collection and map properties and try to design the plugin in a way that treats "empty" as the default.
It is still possible to add further collectors (hence the call to put is allowed) because of the lazy evaluation, but they will have no effect.
That feels weird. What would be the drawback of throwing an exception in that case to make the caller aware that the collector will have no effect?
try to design the plugin in a way that treats "empty" as the default.
The API still feels somewhat inconsistent. I just bumped into this again today:
@get:Optional
@get:Input
abstract val headers: MapProperty<String, String>
Does it make sense to put @Optional up there?
And trying to use a Property<Map<String, String>> fails at runtime with:
Please use the ObjectFactory.mapProperty() method to create a property of type Map<K, V>.
Just bumped into this again today 😅 . A simple workaround would be to allow Property<Map<...>>.
When trying to create such property at the moment it fails with:
Please use the ObjectFactory.mapProperty() method to create a property of type Map<K, V>.
Allowing Property<Map<...>> would be a simple solution for users that don't need per-item collectors but needs to differentiate between empty and unset
Update:
Using mapProperty.convention(null as Map<String, String>?) instead of mapProperty.set(null as Map<String, String>?) works as expected:
- The property will have an unset value.
- Adding items will correctly set the value.
Thanks @octylFractal for pointing at this solution 🙇
I stumbled upon the same problem. Take this script:
abstract class MyTask extends DefaultTask {
@Input
abstract Property<String> getWhoAmI()
@Input
@Optional
abstract ListProperty<String> getStrings();
@TaskAction
void doSomething() {
print("Task $name (list property ${whoAmI.get()}) ")
if (strings.isPresent()) {
println("strings: ${strings.get()}")
} else {
println("strings: not set")
}
}
}
tasks.register("a", MyTask) {
whoAmI = "not set"
}
tasks.register("b", MyTask) {
whoAmI = "set to empty list"
strings.set([])
}
tasks.register("c", MyTask) {
whoAmI = "set to non empty list"
strings.set(["one","two"])
}
tasks.register("check") {
dependsOn(tasks.withType(MyTask))
}
and run gradle check. The output is:
> Task :a
Task a (list property not set) strings: []
> Task :b
Task b (list property set to empty list) strings: []
> Task :c
Task c (list property set to non empty list) strings: [one, two]
Which is annoying because I cannot make the difference between "the user didn't express an opinion" and "the user set to an explicit empty list". Note that this could be fixed differently if there was a convention value and that there was a way to ask if the value is the convention one:
if (strings.usesConventionValue() {
....
}
For the next release, Gradle 8.7, we are introducing alternative verbs for the kind of expectation this issue states:
mapProperty.set(null as Map<String, String>?)
mapProperty.put("key", "value")
mapProperty.isPresent // this is false, but some expect it should be true
Instead of put, use insert.
mapProperty.set(null as Map<String, String>?)
mapProperty.insert("key", "value")
mapProperty.isPresent // this is true
That will be true even if convention is set to a null map (i.e. unset). If the convention is actually defined, it will commit the convention as the explicit value before performing the addition. Also, the property will have a value even if the value being inserted is undefined.
See https://github.com/gradle/gradle/pull/27859, hopefully the javadoc for the new methods will be clear enough. If not, please provide feedback as questions/comments.
That said, I would like to understand better the use cases for checking whether a property has its value explicitly assigned or via convention, as suggested by @melix, possibly as a separate issue. I wonder if such use cases can't really be addressed with API we introduced recently during 8.7. Note that now we do have an internal method for that, but it is meant to help with unit testing the Property implementation.
@martinbonnin and others: any reason why we should not close this as addressed via #27859?
Hi @abstratt ! Thanks for looking into this.
My hunch is that adding more methods is not the solution to this particular problem. Or at least not without deprecating some others. This is a problem about communicating the intent of the API, increasing the API surface will only make things harder IMO.
Re-reading my initial comment, I changed my mind. In retrospect, my preferred expected behaviour right now would be more like so:
Preferred Behaviour after letting it soak some time:
mapProperty.set(null as Map<String, String>?)
// this is not ok as there is no map to put into
mapProperty.put("key", "value") // throws exception: cannot put entry in absent map
This way, users can't just put values into an absent MapProperty, they have to either set everything at once:
// this is ok, set all the entries at once
mapProperty.set(mapOf("key" to "value"))
// this is ok, set all the entries from a provider
mapProperty.set(provider { mapOf("key" to "value") })
Or build their map proressively if they want collecting entries:
// this is ok, set entries one after the other
mapProperty.set(emptyMap<String, String>())
mapProperty.put("key1", provider { "value1" })
As a plugin author, I could distinguish between "empty" (in both the above scenarios) and "absent" (if the user didn't set the property at all). I believe this would also fix @melix issue above.
Something else that I would love to have is consistent initial state:
val listProperty = objects.property(String::class.java)
listProperty.isPresent // false
val mapProperty = objects.mapProperty(String::class.java, String::class.java)
mapProperty.isPresent // currently returns true, feels very inconsistent
I realize this is probably not the answer you were looking for and I also realize the MapProperty ship has sailed and changing such a low level behaviour is probably not an option anymore. But overall, I feel it'd be a much consistent story overall.
PS: as a potential way forward here, how bad would it be to allow properties of type Property<Map<K,V>> (i.e. remove this check)? It'd probably be enough for my use cases without adding too much API surface. Regular Map users could just use the non-collecting/predictable Property<Map<K,V>> and more advanced users go look into the details of MapProperty, put, convention, etc..?
@martinbonnin We do consider deprecating add*() and put*(), but only after append*() and insert*() are out of incubation.
Btw, I cannot reproduce the inconsistent behavior between map and list properties (tried 8.2 and 8.6). If you have a reproducible test case, please open a separate ticket for that.
Now, back to your use case. We would like to promote a model where plugin authors set conventions to their own plugin task properties only, and let users overwrite/build on them via explicit values if they want to. it seems that model is not working for you. Could you help us understand the reason? It may be the case that the API we have now enables use cases that were blocked before.
I cannot reproduce the inconsistent behavior between map and list properties
Apologies, the variable was completely misnamed listProperty instead of stringProperty:
val stringProperty = objects.property(String::class.java)
stringProperty.isPresent // false
val mapProperty = objects.mapProperty(String::class.java, String::class.java)
mapProperty.isPresent // currently returns true, feels very inconsistent
This is with Gradle 8.6 and this seems to be done on purpose (cf doc) although I'm not really clear why.
plugin authors set conventions to their own plugin task properties only, and let users overwrite/build on them via explicit values if they want to
Sounds good 👍
it seems that model is not working for you.
It actually is working right now for me with the solution from https://github.com/gradle/gradle/issues/12222#issuecomment-674741780:
// plugin
mapProperty.convention(null as Map<String, String>?)
// case 1: user does nothing
mapProperty.isPresent // false
// case 2: user sets emptyMap
mapProperty.set(emptyMap())
mapProperty.isPresent // true
// case 3: user calls put on top of the default emptyMap
mapProperty.put("key", "value")
mapProperty.isPresent // true
So I'm technically not blocked and I can distinguish between user null and user absent in my plugin.
Still, I'm having a hard time understanding how this API works.
If the MapProperty is initialized with an empty map then why does the convention even matters? I thought conventions were only useful if a provider was missing a value (which is not the case here because it has an empty list value by default). So this is all surprising. And why is the MapProperty not initialized to absent in the first place?
Maybe this is all complexity required by the fact that MapProperty now has lazy entries and now there are questions like: "what happens if an entry value is missing? Should the whole MapProperty be missing? Or should the entry be ignored?" (it seems to be the former but I had to try it out, I couldn't really guess).
But that complexity isn't always required and not being able to use a Property<Map<K,V>> feels like a missed opportunity for simpler APIs.
@martinbonnin Thanks, I indeed was misled by the var name, sorry for my confusion.
Still, I'm having a hard time understanding how this API works.
Thanks for the feedback, we are trying to improve in that area.
If the MapProperty is initialized with an empty map then why does the convention even matters?
The same strategy you tried would have worked with set((Map) null) instead of convention((Map) null). Both will replace the "initial effective value", the former by unsetting the explicit value, the latter by unsetting the convention.
I thought conventions were only useful if a provider was missing a value (which is not the case here because it has an empty list value by default).
To be more specific, conventions are only supported on properties (and soon, ConfigurableFileCollections), but not in providers in general. The effective value of a collection/map property is:
- an empty collection/map, when instantiated
- the value set with
convention(), if an explicit value is not set withset() - the value set with
set()
So this is all surprising. And why is the MapProperty not initialized to absent in the first place?
List/set/map properties are special as they have three possible states (empty, populated and unset), whereas non-composite properties allow only set and unset. One benefit for collection properties is that they can be used for incremental updates without preliminary setup. If everyone needed to set some value before setting them, then they would risk overwriting a value previously set.
what happens if an entry value is missing? Should the whole MapProperty be missing? Or should the entry be ignored?
As you saw, the entire property value is considered missing, as described here and here.
One benefit for collection properties is that they can be used for incremental updates without preliminary setup.
Would you be open to allowing Property<Map<K,V>> in task properties? I would personally prefer to use that API. It is more inline with what I'm used to and I don't need incremental updates and the cognitive load attached to them.
That said, I would like to understand better the use cases for checking whether a property has its value explicitly assigned or via convention, as suggested by @melix, possibly as a separate issue.
The use case is that the Micronaut plugins build on top of the Native Build Tools plugin. The NBT plugins set some conventions in a map property, and the Micronaut plugins need to add to these conventions. However, if the user has explicitly set some value, we don't want to set our conventions. So we need a way to append to the conventions, vs appending to what is set.
We do consider deprecating add*() and put*(), but only after append*() and insert*() are out of incubation.
I would not do this. It is already extremely painful for plugin authors to keep up to date and support multiple Gradle versions. It is extremely annoying to have new APIs you have to use because they are better, but then you realize that you made a mistake and deprecate with something else. It happened several times in the past years (e.g config cache) which makes it extremely unpleasant to write Gradle plugins.
Would you be open to allowing Property<Map<K,V>> in task properties?
@martinbonnin Could you please open a feature request for property value types such as Map (and I guess List) to be allowed? That is beyond the scope of this issue. I don't have enough context, but Gradle goes out of its way to guide users into using the specifically purposed properties instead of general properties.