search

nhl / link-move

A model-driven dynamically-configurable framework to acquire data from external sources and save it to your database.
Apache License 2.0
35 stars 15 forks source link

MutableExtractorModel.getProperties() should respond unmodifiable hash map since 2.9 #177

Closed vitalz closed 2 years ago

vitalz commented 4 years ago

Since 2.9 version MutableExtractorModel responds copied properties hash map which is modifiable.

https://github.com/nhl/link-move/blob/124ebe01ae988f19b2830b250477db8633ad51c4/link-move/src/main/java/com/nhl/link/move/extractor/model/MutableExtractorModel.java#L34

Imagine there is a customer link move extension which uses this method to add some properties. This plugin doesn't know (directly) about link move version what is causing compatibility issues, i.e since 2.9 a following piece of code runs successfully and will never fail fast:

.getProperties().put("prop1", "value1");

But it would be much better if unmodifiable map is responded like:

return Collections.unmodifiableMap(singleValueProperties);

Then code like .getProperties().put("prop1", "value1") fails fast and a user will easily figure out that addProperty(String name, String value) has to be used.

andrus commented 4 years ago

While I agree in principal, this is mostly moot, as ExtractorModel.getProperties() is deprecated and will be removed eventually. Comment from the deprecation docs:

@deprecated since 2.9 as we switched to multi-value properties. 
Use getPropertyValue(String) or #getPropertyValues(String)} instead.
vitalz commented 4 years ago

For now while the method is presented and code is still compiled @Deprecated annotation doesn't guarantee fail fast which forces customers to change their code immediately in a right place in their extension - otherwise they would be debugging (sometimes for a long time) in order to figure out where a problem was which had been hidden from them (and also they should not care of encapsulated link move implementation in the detail).

andrus commented 2 years ago

Sorry, it took a while, but in 3.0 the API in question is finally removed per #216, and the new multi-value property API is made immutable.