Skip to content

fix issues with Void return type due to rxjs map operator null checks #236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
David-Development wants to merge 2 commits into from
Closed

fix issues with Void return type due to rxjs map operator null checks #236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95ced68
fix issues with Void return type due to rxjs map operator null checks
David-Development Aug 8, 2020
540ada0
update unit tests / return empty object if type not specified
David-Development Aug 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 6 additions & 25 deletions src/main/java/com/nextcloud/android/sso/api/NextcloudAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import java.lang.annotation.Documented;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;
import java.lang.reflect.Constructor;
import java.lang.reflect.Type;

import io.reactivex.Observable;
Expand All @@ -46,18 +45,6 @@ public class NextcloudAPI {

private static final String TAG = NextcloudAPI.class.getCanonicalName();

private static final Void NOTHING = getVoidInstance();

private static Void getVoidInstance() {
Constructor<Void> constructor = (Constructor<Void>) Void.class.getDeclaredConstructors()[0];
constructor.setAccessible(true);
try {
return constructor.newInstance();
} catch (Exception e) {
throw new IllegalStateException("Should never happen, but did: unable to instantiate Void");
}
}

private NetworkRequest networkRequest;
private Gson gson;

Expand Down Expand Up @@ -127,22 +114,16 @@ public <T> T performRequestV2(final @NonNull Type type, NextcloudRequest request
return convertStreamToTargetEntity(response.getBody(), type);
}

private <T> T convertStreamToTargetEntity(InputStream inputStream, Type targetEntity) throws IOException {
T result = null;
try (InputStream os = inputStream;
Reader targetReader = new InputStreamReader(os)) {
private <T> T convertStreamToTargetEntity(InputStream responseStream, Type targetEntity) throws IOException {
try (Reader targetReader = new InputStreamReader(responseStream)) {
if (targetEntity != Void.class) {
result = gson.fromJson(targetReader, targetEntity);
/*
if (result != null) {
Log.d(TAG, result.toString());
}
*/
return gson.fromJson(targetReader, targetEntity);
} else {
Copy link
Contributor

@desperateCoder desperateCoder Aug 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

@David-Development David-Development Aug 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙈

Copy link
Contributor

@desperateCoder desperateCoder Aug 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@David-Development David-Development Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Contributor

@desperateCoder desperateCoder Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

result = (T) NOTHING;
// If the developer doesn't tell us what return type he wants.. there is nothing
// we can do.. so we'll just return an empty object
return (T) new Object();
}
}
return result;
}


Expand Down
6 changes: 3 additions & 3 deletions src/main/java/com/nextcloud/android/sso/api/NextcloudRetrofitServiceMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public T invoke(NextcloudAPI nextcloudAPI, Object[] args) throws Exception {
rBuilder.setUrl(url.replace(varName, String.valueOf(args[i])));
} else if(annotation instanceof Header) {
Object value = args[i];
String key =((Header) annotation).value();
String key = ((Header) annotation).value();
addHeader(rBuilder, key, value);
} else if(annotation instanceof FieldMap) {
if(args[i] != null) {
Expand Down Expand Up @@ -188,13 +188,13 @@ public T invoke(NextcloudAPI nextcloudAPI, Object[] args) throws Exception {
}
//fallback
return (T) nextcloudAPI.performRequestObservableV2(typeArgument, request).map(r -> r.getResponse());

} else if(ownerType == Call.class) {
Type typeArgument = type.getActualTypeArguments()[0];
return (T) Retrofit2Helper.WrapInCall(nextcloudAPI, request, typeArgument);
}
} else if(this.returnType == Observable.class) {
return (T) nextcloudAPI.performRequestObservableV2(Object.class, request).map(r -> r.getResponse());
// Observables without any type information (see above for Observables with type info)
return (T) nextcloudAPI.performRequestObservableV2(Void.class, request).map(r -> r.getResponse());
Copy link
Contributor

@desperateCoder desperateCoder Aug 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we go, here is the ClassCastException going to happen

} else if (this.returnType == Completable.class) {
return (T) ReactivexHelper.wrapInCompletable(nextcloudAPI, request);
}
Expand Down
12 changes: 12 additions & 0 deletions src/test/java/com/nextcloud/android/sso/api/API.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,18 @@


public interface API {
@GET("callWithNoReturnType")
Call callWithNoReturnType();

@GET("getWithNoReturnType")
Observable getWithNoReturnType();

@GET("getListWithNoType")
Observable<List> getListWithNoType();

@GET("getWithVoidReturnType")
Observable<Void> getWithVoidReturnType();

@GET("version")
Observable<String> getRequest();

Expand Down
41 changes: 40 additions & 1 deletion src/test/java/com/nextcloud/android/sso/api/TestRetrofitAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import static org.mockito.Mockito.lenient;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

public class TestRetrofitAPI {

Expand Down Expand Up @@ -64,6 +63,16 @@ public void setUp() {
mApi = new NextcloudRetrofitApiBuilder(nextcloudApiMock, mApiEndpoint).create(API.class);
}

@Test
public void callWithNoReturnType() throws Exception {
mApi.callWithNoReturnType();
NextcloudRequest request = new NextcloudRequest.Builder()
.setMethod("GET")
.setUrl(mApiEndpoint + "callWithNoReturnType")
.build();
verify(nextcloudApiMock).performRequestV2(eq(retrofit2.Call.class), eq(request));
Copy link
Contributor

@desperateCoder desperateCoder Aug 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@David-Development David-Development Aug 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@desperateCoder desperateCoder Aug 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

@David-Development David-Development Aug 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}

@Test
public void getRequest() {
mApi.getRequest();
Expand All @@ -74,6 +83,36 @@ public void getRequest() {
verify(nextcloudApiMock).performRequestObservableV2(eq(String.class), eq(request));
}

@Test
public void getListWithNoType() {
mApi.getListWithNoType();
NextcloudRequest request = new NextcloudRequest.Builder()
.setMethod("GET")
.setUrl(mApiEndpoint + "getListWithNoType")
.build();
verify(nextcloudApiMock).performRequestObservableV2(eq(List.class), eq(request));
}

@Test
public void getWithNoReturnType() {
mApi.getWithNoReturnType();
NextcloudRequest request = new NextcloudRequest.Builder()
.setMethod("GET")
.setUrl(mApiEndpoint + "getWithNoReturnType")
.build();
verify(nextcloudApiMock).performRequestObservableV2(eq(Void.class), eq(request));
}

@Test
public void getWithVoidReturnType() {
mApi.getWithVoidReturnType();
NextcloudRequest request = new NextcloudRequest.Builder()
.setMethod("GET")
.setUrl(mApiEndpoint + "getWithVoidReturnType")
.build();
verify(nextcloudApiMock).performRequestObservableV2(eq(Void.class), eq(request));
}

@Test
public void getFolders() {
mApi.getFolders();
Expand Down