Fix "setreplace" using a FileMap (#381)

[dr0i]

"AbstractReadOnlyMap" didn't allow access to the full contents of Maps. It allowed only access by giving a key. While this makes sense when e.g. querying (big) databases this broke the "setreplace" function where the whole Map is loaded first to be able to replace (parts of) the input string.

[dr0i]

This changes all reading methods of 'AbstractReadOnlyMap' to not be final so that they can be overidden by extending classes. This overriding is needed in FileMap (well, just one of these methods but I decided to change all reading methods to not be final to be more consistent - if this makes sense) to make it working with the morph function setreplace map=.... I think this is not an API break. If you have an other opinion we could implement this fix in an other way, for example by new class from which FileMap could extend.

[blackwinter]

This changes all reading methods of 'AbstractReadOnlyMap' to not be final

You skipped containsKey(). Was this intentional?

I think this is not an API break.

I have to disagree, unfortunately. It breaks the "read-only" property. I'm not a fan of final methods/classes by any means, but it seems to serve a purpose here.

[dr0i]

You skipped containsKey(). Was this intentional?

Yes, because it makes use of get(final Object key) which is not itself defined in AbstractReadOnlyMap but must be implemented by the extending classes. So there is no need for any implementer to override containsKey() (it was also the only method doing anything other than throwing UnsupportedOperationException).

It breaks the "read-only" property. I'm not a fan of final methods/classes by any means, but it seems to serve a purpose here.

Hm, agreed that it is meaningful in some way. What I meant by "not an API break" is that these changes should not introduce any backward incompatibility. Maybe I am wrong here so I appreciate your input very much. For me it looks like "in the past it was forbidden to override some methods" , and thus no such overriding methods were implemented (say e.g. by the JndiSqlMap). No you could (not must) do that. Agreed that if someone would implement the entrySet() in JndiSqlMap that could lead to problems if someone would want to use the setreplace map function (Memory issue comes to mind). But no existing usages of JndiSqlMap (or any other own implementations making use of AbstractReadOnlyMap) could atm use the setreplace map because atm it throws UnsupportedOperationException. There are many ifs , but one could never know... How would you fix setreplace map? My first approach was to write a new class AbstractReadOnlyMapWithAccessToFullContent and let the FileMap extend that new class. Mind then, though, that the main problem (the possible Memory issue) could also happen with a big file (if someone uses the setreplace map function. Or would it be enough to just write some javadoc warning to implement some of the methods because of possible MemoryExceptions?

[blackwinter]

So there is no need for any implementer to override containsKey()

I agree that it's not crucial or anything, but a concrete implementation might care about efficiency (e.g., by delegating to Map.containsKey() or SQL EXISTS).

How would you fix [it]?

Generally, I haven't thought much about the issue at hand. But off the top of my head, FileMap could restore read-only behaviour by returning an immutable Set instead. I don't think there's a way for AbstractReadOnlyMap to impose this constraint on subclasses, though.

[blackwinter]

FileMap could restore read-only behaviour by returning an immutable Set instead.

Oh, and the Entrys have to be made immutable as well.

[dr0i]

I agree that it's not crucial or anything, but a concrete implementation might care about efficiency (e.g., by delegating to Map.containsKey() or SQL EXISTS).

Agreed. Will remove the final here, too.

FileMap could restore read-only behaviour by returning an immutable Set instead.

I don't understand. AbstractReadOnlyMap still guarantees a read-only Map (all write API still throws UnsupportedOperationException).

[blackwinter]

I don't understand. AbstractReadOnlyMap still guarantees a read-only Map

No, once you expose its entrySet() it can be modified. E.g., fileMap.entrySet().clear() :grin:

A possible alternative might be to change SetReplacer.addReplacements() to iterate over the keySet() instead (which would still have to be returned as an unmodifiable Set in FileMap).

[dr0i]

... it can be modified.

True. What if we use an "unmodifiable Map" like in https://github.com/metafacture/metafacture-core/pull/418 ?

[blackwinter]

What if we use an "unmodifiable Map" like in #418 ?

See the discussion over there.