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

Incorrect logic with ReflectionUtils.getAllFields() method - wrong order of fields #73

Closed
HitOdessit opened this Issue Sep 1, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@HitOdessit

HitOdessit commented Sep 1, 2015

Hi Stuart,

Recently I've stumbled an interesting issue - if you save some data into database file and load it later on the same Android OS version (read same JVM) everything works as expected. But if you load that .db file on another Android OS version, data can't be loaded, lots of crashes.

I've made some research on this issue and found root cause - your logic in ReflectionClassLoader .loadClass(...):104 method assumes that utility method ReflectionUtils.getAllFields() returns list of class fields always in the same order. But this is not the case. Quote from Java API:
http://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#getFields()

Returns an array containing Field objects reflecting all the accessible public fields of the class or interface represented by this Class object. 
The elements in the array returned are not sorted and are not in any particular order. 
This method returns an array of length 0 if the class or interface has no accessible public fields, or if it represents an array class, a primitive type, or void.

So, I think logic of using ReflectionUtils.getAllFields() method should be changed.

@Stuart-campbell

This comment has been minimized.

Show comment
Hide comment
@Stuart-campbell

Stuart-campbell Sep 1, 2015

Owner

Good point, had not thought about database cross device compatibility.

Not a problem to sort fields alphabetically, only problem it is would break it for everyone else.

Maybe have an option to do so in the config with the default not.

Thanks

Owner

Stuart-campbell commented Sep 1, 2015

Good point, had not thought about database cross device compatibility.

Not a problem to sort fields alphabetically, only problem it is would break it for everyone else.

Maybe have an option to do so in the config with the default not.

Thanks

@HitOdessit

This comment has been minimized.

Show comment
Hide comment
@HitOdessit

HitOdessit Sep 10, 2015

Hi Stuart, any updates on this issue?
For me, it looks like a major issue, 'cos user simply need to update OS version on his device and any app based on RushORM simply will break. Pretty critical, as for me.

HitOdessit commented Sep 10, 2015

Hi Stuart, any updates on this issue?
For me, it looks like a major issue, 'cos user simply need to update OS version on his device and any app based on RushORM simply will break. Pretty critical, as for me.

@Stuart-campbell

This comment has been minimized.

Show comment
Hide comment
@Stuart-campbell

Stuart-campbell Sep 10, 2015

Owner

Hi,

Sorry been meaning to get round to it.
I will get it fixed this weekend.

Thanks

Owner

Stuart-campbell commented Sep 10, 2015

Hi,

Sorry been meaning to get round to it.
I will get it fixed this weekend.

Thanks

Stuart-campbell pushed a commit that referenced this issue Sep 13, 2015

@Stuart-campbell

This comment has been minimized.

Show comment
Hide comment
@Stuart-campbell

Stuart-campbell Sep 13, 2015

Owner

Just updated to v1.1.10 where you can add the following meta-data to the manifest

<meta-data android:name="Rush_order_alphabetically" android:value="true" />

This should order all the fields so will be cross device compatible.

Thanks

Owner

Stuart-campbell commented Sep 13, 2015

Just updated to v1.1.10 where you can add the following meta-data to the manifest

<meta-data android:name="Rush_order_alphabetically" android:value="true" />

This should order all the fields so will be cross device compatible.

Thanks

@nmoskalenko

This comment has been minimized.

Show comment
Hide comment
@nmoskalenko

nmoskalenko Sep 14, 2015

@Stuart-campbell , as I understood form your commit 6e4609a you specified this behavior only for JSON serialization/deserialization purposes ? Or I misunderstood?
This is not the case.
What we need is the availability to use some database on different devices natively.
For example generate database on emulator, copy to real device and use via RushORM without any additional manipulations.

nmoskalenko commented Sep 14, 2015

@Stuart-campbell , as I understood form your commit 6e4609a you specified this behavior only for JSON serialization/deserialization purposes ? Or I misunderstood?
This is not the case.
What we need is the availability to use some database on different devices natively.
For example generate database on emulator, copy to real device and use via RushORM without any additional manipulations.

@Stuart-campbell

This comment has been minimized.

Show comment
Hide comment
@Stuart-campbell

Stuart-campbell Sep 14, 2015

Owner

Hi,

The key to that commit is the change in Subproject commit number.

All fields can now be order alphabetically into the database not just json. I only added it to the json for consistency, I understand an ordered json is pretty useless.

Here is a link to the changes that should fix your issue be making the order of getAllFields the same regardless of device Stuart-campbell/RushCore@70dbe6e

Thanks

Owner

Stuart-campbell commented Sep 14, 2015

Hi,

The key to that commit is the change in Subproject commit number.

All fields can now be order alphabetically into the database not just json. I only added it to the json for consistency, I understand an ordered json is pretty useless.

Here is a link to the changes that should fix your issue be making the order of getAllFields the same regardless of device Stuart-campbell/RushCore@70dbe6e

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment