CASL-1785: fix HttpStorageProperties bug#575
Conversation
Signed-off-by: Marcin-Here <ext-marcin.grabowski@here.com>
Code Coverage
|
| if (raw instanceof HeaderMap) { | ||
| return (HeaderMap) raw; | ||
| } | ||
| if (raw instanceof Map<?, ?>) { |
There was a problem hiding this comment.
IntelliJ complains that it is always false. Meaning, it will never to Map<?, ?> . Which seems true. Pls revisit and remove this condition if not needed.
In that case, the earlier check of raw instanceof HeaderMap is also not needed. It will either be null or of type HeaderMap, so just checking for null should be sufficient?
There was a problem hiding this comment.
Personally, I vote for keep these checks. The Kotlin Multiplatform build is not the most straightforward when it comes to IDE errors/warnings as I have seen.
In this case for example I can wrongly do setRaw(HEADERS, "12"); somewhere, and that string would still be stored. So correcting the headers here at getter looks to be the only solution without interfering with the underlying MapProxy (ideally I prefer to somehow catch this right when it was set).
There was a problem hiding this comment.
The way I am thinking, creates a doubt - if there is a possibility where setRaw(HEADERS, "12"); can happen during runtime? In the code, I don't see such case. Help with one example?
There was a problem hiding this comment.
One example would be new HttpStorageProperties().setRaw(<<any Object>>), by mistake somewhere.
There was a problem hiding this comment.
I think you both are correct. with setRaw() and getRaw() we lose encapsulation but it's design of proxy model. Also Map check looks like a lot of boilerplate, and in normal flow it would be unnecessary, also Intelij is sometimes confused by proxy model. What is happening in that case: In most used flow in Storage, Handler etc jsons. When we want access properties we use JvmBoxingUtil.box(eventHandlerConfig.getProperties(), DefaultStorageHandlerProperties.class)); and in our case HttpStorageProperties storageProperties = JvmBoxingUtil.box(storage.getProperties(), HttpStorageProperties.class); Now when properties has nested objects like in our case Map. In boxing process it's become AnyObject by proxy model. In normal use case we then just call gets on our properties, if we don't have Map<?,?> check in get. It would fail HeaderMap check and create and return default headers, so we need step to translate our AnyObject to our wrapper in this case HeaderMap extends JvmMapProxy<String, String> . I personally think and from I remember Kuba agreed, that proxy model it's a bit complicated and confusing to use for new users, also creating what normally would be POJO becomes complicated and time consuming tasks, like in this example of which we're talking right now.
There was a problem hiding this comment.
Also added test shouldPreserveHeadersWhenBoxingStoragePropertiesFromJson to help understand what is going on.
|
|
||
| public void setProtocol(final HttpInterface protocol) {setRaw(HTTP_INTERFACE, protocol);} | ||
|
|
||
| private @NotNull HeaderMap toHeaderMap(@NotNull Map<?, ?> rawHeaders) { |
There was a problem hiding this comment.
If the earlier comment about "Map<? , ?> not being used" is accepted, then this function also can be removed?
|
|
||
| # When updating the version, please consider: | ||
| # automatically updated: | ||
| # here-naksha-lib-core/NakshaVersion (static property: latest) |
There was a problem hiding this comment.
I think NakshaVersion also needs to be updated with new version?
Signed-off-by: Marcin-Here <ext-marcin.grabowski@here.com>
Code Coverage
|
3 participants
No description provided.