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

Remember instances for annotated entities too #254

Closed

Conversation

jivimberg
Copy link
Contributor

This solves issues: #215 and #243 as well as #185.
Basically I'm using a Map with weak keys to keep track of the row ID for a given object instance. In the past the id field was used for the entities that extend SugarRecord, but that's not possible for annotated entities.

Conflicts:
	library/build.gradle
	sugartestapp/src/androidTest/java/me/jivimberg/android/sugartestapp/robolectric/SugarRecordTest.java
	sugartestapp/src/main/java/me/jivimberg/android/sugartestapp/model/annotated/Car.java
Conflicts:
	library/src/com/orm/SugarRecord.java
@whoshuu whoshuu added this to the 1.4.0 milestone Apr 4, 2015
@whoshuu
Copy link
Collaborator

whoshuu commented Apr 4, 2015

I'm going to take a look at this tomorrow. This is a pretty big bug that should be fixed!

@jivimberg
Copy link
Contributor Author

The fix is pretty straightforward. The only thing is that I had to add the Guava dependency to create the concurrent map with weak keys to avoid a memory leak.
Let me know if I can be of any help.

@whoshuu
Copy link
Collaborator

whoshuu commented Apr 7, 2015

I'm close to merging this in, but it's failing an important case:

Model model = new Model("Test");
long id = SugarRecord.save(model);
Model query = SugarRecord.findById(Model.class, 1);
assertEquals(id, new Long(query));

The expectation is that the queried model retains the id from the original, but Java sees model and query as different objects. One was created by the user, the other was inflated by Sugar.

@whoshuu whoshuu closed this in 1f49d7c Apr 7, 2015
@jivimberg jivimberg deleted the upstreamAttachedRecords branch May 11, 2015 02:59
@sibelius
Copy link
Contributor

@jivimberg do you have any ideia of how to remove guava dependency?

Another problem is that MapMaker is deprecated (https://github.com/google/guava/wiki/MapMakerMigration)

is there no way to fix this issue without using a Map?

@jivimberg
Copy link
Contributor Author

It seams that the MapMaker call can be replaced by a CacheBuilder which still has the option of specifying weakKeys

@sibelius
Copy link
Contributor

@jivimberg the problem with CacheBuilder is that it has guava dependency that make the size of this lib bigger, can we just add part of guava ?

@jivimberg
Copy link
Contributor Author

Oh! I see. I guess we could easily take a look at the CacheBuilder implementation. The key thing here is to have weak reference for the keys in the map so we don't run into a memory leak

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

Successfully merging this pull request may close these issues.

3 participants