fix issues with Void return type due to rxjs map operator null checks#236
fix issues with Void return type due to rxjs map operator null checks#236David-Development wants to merge 2 commits intomainfrom
Conversation
|
This one will still die (in most cases my code comes across this line...): |
| .setMethod("GET") | ||
| .setUrl(mApiEndpoint + "callWithNoReturnType") | ||
| .build(); | ||
| verify(nextcloudApiMock).performRequestV2(eq(retrofit2.Call.class), eq(request)); |
There was a problem hiding this comment.
Doesn't this just verify the call is made? The problem is returning the async result, so I'm not sure if this a viable test... Does this test break if you bring the map() back?
There was a problem hiding this comment.
Yes, that is indeed a problem. The data is coming from a mock and therefore the observable always returns an empty observable. If we want to add more checks I think we'd have to add dummy objects for all types.. right?
There was a problem hiding this comment.
I think a mocked test isn't the way to go here, at least we are mocking the wrong objects here... The mock would need to be much deeper, somewhere at the NC Files app interface I guess. We need a mock which instantly triggers the callbacks to see, if anything dies in this chain, but I didn't dig in this far yet. I'll have a look!
There was a problem hiding this comment.
I agree, the tests here are mostly to check that the type and url was extracted correctly and that the interface was called. However return types are not considered. Thank you for looking into it!
Codacy32Lint
SpotBugs (new)
SpotBugs (master)
|
| try (Reader targetReader = new InputStreamReader(responseStream)) { | ||
| if (targetEntity != Void.class) { | ||
| return gson.fromJson(targetReader, targetEntity); | ||
| } else { |
There was a problem hiding this comment.
This is a safe ClassCastException, since we are casting the result... Correct me if I'm wrong, but i think you cast the Object somewhere to Void, because I definitely don't. I used to get, what I defined I want to get, so the casting is done SSO-Internally, see comment below...
There was a problem hiding this comment.
Ugh.. me being naive while thinking that I could just throw in some typescript-style code 🤷♂️ 😅 I think you might be right about the casting thing. However it would be interesting to see if it actually fails 🙈
There was a problem hiding this comment.
No need to be curious here, i mean look: you have a check above, if the target entity ( = T) is NOT a Void. Then, in else the first thing you do is casting a new Object() to T ( = Void). One thing I can tell you for sure: Object.class != Void.class. Game over. Thats why I was going for NOTHING, since this is at least semi-safe, as long as the caller specifies a return type. Otherwise the worst thing that could happen is, when the return type is specified, but the server doesn't return anything (=null), then we would get the error from the mapper.
There was a problem hiding this comment.
Hm yes, I get that point. I think I'm also good with the nothing option then. However I'm just thinking about other case. So if you specify List without a type, it'll be interpreted as Void as well, right? But what if I'm too lazy as a programmer to write the type but I still want to somehow receive my response object..? Maybe I'm overthinking it a little here..
There was a problem hiding this comment.
So if you specify List without a type, it'll be interpreted as Void as well, right? But what if I'm too lazy as a programmer to write the type but I still want to somehow receive my response object..?
Nope, would be a List<Object> you could cast, if your're brave enough to guess the actual type. Would work in this case. The problem with your solution is only the void type, which is broken here. So we're back at the beginning of our problem 🤣. I'll have another look at it, maybe this weekend and see what I can do about this.
| } else if(this.returnType == Observable.class) { | ||
| // Observables without any type information (see above for Observables with type info) | ||
| return (T) nextcloudAPI.performRequestObservableV2(Void.class, request); //.map(r -> r.getResponse()); | ||
| return (T) nextcloudAPI.performRequestObservableV2(Void.class, request).map(r -> r.getResponse()); |
There was a problem hiding this comment.
here we go, here is the ClassCastException going to happen
|
@desperateCoder Did you have a chance to fight the unit tests yet? 🙈 Or do you prefer that we close this pr here and create a new issue instead? |
|
@David-Development Actually not, since @stefan-niedermann keeps throwing feature requests at my head... If you say you really need my help, I'll see what I can do, although I hate testing 😅 |
|
@David-Development @desperateCoder @stefan-niedermann I guess this PR is obsolete given the lately merged new approach brought in by Stefan via #542 |
|
obsolete from my PoV |
6 participants
No description provided.