Skip to content
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.

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

feat: MLIBZ-2447 Extending user class with properties of different class cause an exception #167

Conversation

@yuliya-guseva
Copy link
Contributor

commented May 11, 2018

Description

Extending user class with properties of different class cause an exception

Changes

Updated auth response parsing. Now the SDK gets user class as a model for response parsing. Previously the model for response parsing had only id and _kmd fields, other primitive fields were got as fields from GenericJson and it didn't work for GenericJson fields in the user model class.

Tests

Instrumented

@yuliya-guseva yuliya-guseva requested a review from vinaygahlawat May 11, 2018

@codecov-io

This comment has been minimized.

Copy link

commented May 11, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (indev@2e2bb1e). Click here to learn what that means.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##             indev    #167   +/-   ##
=======================================
  Coverage         ?   58.1%           
  Complexity       ?     472           
=======================================
  Files            ?      41           
  Lines            ?    3132           
  Branches         ?     478           
=======================================
  Hits             ?    1820           
  Misses           ?    1165           
  Partials         ?     147
Impacted Files Coverage Δ Complexity Δ
...d-lib/src/main/java/com/kinvey/android/Client.java 52.86% <100%> (ø) 16 <0> (?)
.../main/java/com/kinvey/android/store/UserStore.java 48.05% <43.47%> (ø) 23 <8> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e2bb1e...503d962. Read the comment docs.

@@ -431,7 +431,7 @@ public Context getContext(){
* <p/>
* This Builder class is not thread-safe.
*/
public static class Builder extends AbstractClient.Builder {
public static class Builder<T extends User> extends AbstractClient.Builder {

This comment has been minimized.

Copy link
@vinaygahlawat

vinaygahlawat May 29, 2018

Contributor

Will this be a breaking change to the client builder?

This comment has been minimized.

Copy link
@yuliya-guseva

yuliya-guseva May 31, 2018

Author Contributor

No, It won't be breaking change.

public Class getUserClass(){
return this.userModelClass;
public Class<T> getUserClass(){
return this.userModelClass != null ? this.userModelClass : BaseUser.class;

This comment has been minimized.

Copy link
@vinaygahlawat

vinaygahlawat May 29, 2018

Contributor

Why have this logic here instead of defaulting userModelClass to BaseUser.class?

This comment has been minimized.

Copy link
@yuliya-guseva

yuliya-guseva May 31, 2018

Author Contributor

It was done because getUserClass must return Class<T> type, but BaseUser.class should be cast to Class<T> before. I think I can return old implementation for getUserClass and add casting userModelClass = (Class<T>) BaseUser.class. It will be more expectable for users. Updated.


}
@Key
private InternalUserEntity internalUserEntity;

This comment has been minimized.

Copy link
@vinaygahlawat

vinaygahlawat May 29, 2018

Contributor

Do we have a test case where the User class is extended?

@vinaygahlawat
Copy link
Contributor

left a comment

LGTM

@yuliya-guseva yuliya-guseva merged commit 256a946 into indev Jun 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yuliya-guseva yuliya-guseva deleted the feature/MLIBZ-2447_Extending_User_class_with_properties_of_different_class_cause_an_exception branch Jun 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.