realm / realm-java

Realm is a mobile database: a replacement for SQLite & ORMs
http://realm.io
Apache License 2.0
11.46k stars 1.75k forks source link

Support MutableRealmInteger #4266

Closed cmelchior closed 7 years ago

cmelchior commented 7 years ago

(The original initial comment for this issue is here. This heading reflects the current understanding of the project)

The basic semantics for this feature, now called MutableRealmInteger have finally been sorted out here.

This issue now tracks implementation.

TODO:

nirinchev commented 7 years ago

I'm thinking about the following .NET API:

class RealmInteger
{
    void Increment(long value);
    // implicit conversion, comparison operators to long
}

class NullableRealmInteger
{
    bool HasValue { get; }
    long Value { get; }
    void Increment(long value);

    // implicit conversion operators to long?
}

class Party : RealmObject
{
    // Implicit [Required]
    RealmInteger Guests { get; set; }

    NullableRealmInteger NullableGuests { get; set; } // Always return NullableRealmInteger, even if null
}

// Usage:
var party = realm.Find<Party>(1);
Console.WriteLine(party.Guests); // 0

party.Guests = 5; // 5
party.Guests.Increment(3); // 8
party.Guests.Increment(-2); // 6

var guests = party.Guests;
guests.Increment(); // Allowed
party.Guests; // 7

// For the nullable case:
party.NullableGuests; // null
party.NullableGuests.HasValue; // false
party.NullableGuests.Value; // exception
party.NullableGuests.Increment(); // exception
party.NullableGuests = 4;
party.NullableGuests.HasValue; // true
party.NullableGuests.Value; // 4
party.NullableGuests.Increment(); // 5

var nullableGuests = party.NullableGuests;
nullableGuests.Increment(); // Allowed
party.NullableGuests; // 6
party.NullableGuests = null;
nullableGuests.HasValue; // false
nullableGuests.Increment(); // exception

I'm still playing with the API and trying to find a way to merge the two types, but it feels like it won't be very intuitive and nice to use - the main problem being that if it supports null, we can't provide implicit conversion operators to long, which means people won't be able to pass it to methods expecting long without casting, even if they mark it [Required].

cmelchior commented 7 years ago

@zaki50

oldManaged1Value.longValue();// throws IllegalStateException since the oldManaged1Value is invalid. same with deleted RealmObject

Is what bothers me, because not having a method to check for this sounds really bad to me. This indicate that you should really have a RealmInteger.isNull() method. But on the other side that seems really odd if you have

@Required RealmInteger guests;

I guess having NullableRealmInteger and RealmInteger like @nirinchev says, would fix that, but it is a departure from what we have done so far, so I would really like to avoid that, to be honest. Especially since we already have the @Required annotation.

Without adding a completely new type like RealmCounter, the least clunky I have come up with so far is adding RealmInteger.isNull()/RealmInteger.setNull() methods. It would still allow you to make the field @Required and then you can use the two extra methods if you want. Same as we have e.g isValid() on un-managed RealmObject.

zaki50 commented 7 years ago

I'm thinking that RealmInteger should have isValid() instead of isNull().

In most cases, I guess users get RealmInteger instance right before they use it and they don't need to check if it's valid since if invalid, null should be returned instead of RealmInteger.

When users keep RealmInteger reference elsewhere, they still need to check if it's valid even if we prohibit nullify the RealmInteger field since the owner object can be deleted. We can't avoid this validity check in either way.

cmelchior commented 7 years ago

It already has isValid() which indicate if it deleted or not. Overriding that meaning would be rather unfortunate I think. If it can be nullable, it can both have the value null and be managed at the same time.

E.g

realm.beginTransaction()
Party p = realm.createObject(Party.class);
RealmInteger ref = p.guests;
p.guests = null;
ref.isValid(); // true, because party is not deleted
ref.isNull(); // true, because value is null

Treating RealmInteger as always required would get rid of that, but I would rather avoid that since that is an artificial restriction.

zaki50 commented 7 years ago

isValid() indicates that the underlying data is usable or not. I don't think it should reflect that the container object is usable or not. When a field becomes null, I treat it that the counter is destroyed and it's OK for isValid() to return false. And when a new non-null value is set to a null field, I treat it that a new counter is created. That's why the RealmInteger instance is re-created in that case.

Doesn't it make sense?

cmelchior commented 7 years ago

This is the javadoc for it https://github.com/realm/realm-java/blob/master/realm/realm-library/src/main/java/io/realm/internal/ManagableObject.java#L32

I could probably be convinced that we can use isValid() to indicate a null value. Although I still think there are cases where an isValid() and isNull() would return different results. If those cases are important is another matter.

So I assume you want to just have isValid() and = as the way of setting null then?

party.guests.isValid(); // true
RealmInteger ref = party.guests;
party.guests = null;
ref.isValid(); // false 
zaki50 commented 7 years ago

Users can check if the field is null by party.guests == null. As you said, if there is a case that requires to check it only with RealmInteger instance, that's a matter. I don't think any proposal solves this though.

So I assume you want to just have isValid() and = as the way of setting null then?

Exactly!

bmeike commented 7 years ago

@simonask Sigh. Really looking forward to being able to discuss this without an 8-hr delay, next week. I think you are missing the point. This doesn't have anything to do with nulling the value to which the RealmInteger points. This is about whether you can null the pointer to the RealmInteger. There is no equivalent concept in core.

bmeike commented 7 years ago

@cmelchior If core support nullable integers (meaning, roughly, "no data available"), then I absolutely agree, that c.guests.set(null) is consistent. All that takes is having RealmInteger.set() take a Long, instead of a long. Absolutely no problem. I see only one way that c.guests = null makes sense, and that is if we translate it into c.guests.set(null). And that works for me, as long as we can live with the following wildly un-Java-like behavior:

c.guest = null
RealmInteger cGuest = null
cGuest == c.guest // false

I like the idea that a Realm{Counter/Integer} is a RealmObject. I also don't see why it couldn't be @Required. In fact that is almost precisely the implementation for which every has been arguing for the last week. There are two differences:

  1. The implementation to which you all have been pushing me hides the RealmCounter object by translating assignments to the containing object field into assignments to the counter value
  2. In this version the user has control over the nullability of the field that refers to the RealmCounter. Flexibility is the last refuge of scoundrels but at least this would allow them to decide.
cmelchior commented 7 years ago

You mean because it would be like saying null == null; // false? Given what other trade-offs we are debating currently, I could probably live with that.

bmeike commented 7 years ago

Yes. Exactly. That totally weirds me out. It does not, however, weird out either you or Alexsander, and that works for me! ;-)

cmelchior commented 7 years ago

It does weird me out, but since I cannot get everything I want :man_shrugging: 😄

bmeike commented 7 years ago

@beeender Your argument about RealmLists is quite apropos. So what happens when I write, in my code:

class C extends RealmObject {
    private RealmList foo;
    { foo = null; }
}

... and when you say "Well! That's not allowed!", please go have a chat with @zaki50. I'll be right here. ;-P

I think the thing that it seems to me that both of you are missing is that it IS a reference to another object! There is nothing you can do about that. We are defining a type. Sadly, unlike Haskell, Scala, Swift, or a bunch of other languages, Java does not have type aliases. We can hide the fact that it is a reference some of the time. We cannot hide the fact that it is a reference all of the time.

bmeike commented 7 years ago

I think there is a great deal of confusion about the difference between the reference to the counter object and the value of the counter object. The attempts to conflate them might work, but lead to some inconsistencies that, seem to me, to suck. But they do not suck enough so that I am going to put up any further resistance.

I think that Alexander and Christian are advocating a solution that essentially hides the references to the RealmInteger in the context of a RealmObject. They cannot hide it outside the RealmObject. That's easy to say, easy to describe and easy to document. It will surprise people but we can say "I tole you so".

I think that @zaki50 is sort of headed in the same direction but even less consistent:

// after a field becomes null
RealmInteger oldManaged1Value = managed1.realmInteger;
managed1.realmInteger = null; // internally cached RealmInteger is disposed.
managed1.realmInteger // returns null
oldManaged1Value.longValue();// throws IllegalStateException since the oldManaged1Value is invalid. same with deleted RealmObject

WTF? So, how do I find out if I have set the value of managed1.realmInteger to null . Like this???

managed1.realmInteger.isNull() // NPE!

I'm going to continue working towards the hidden RealmInteger implementation... but very very carefully. Hopefully in-person discussions in CPH will help.

I would like, also, to take this moment, to mention that it would, absolutely, be possible to have this discussion -- perhaps, even, to bring it to some kind of conclusion -- before implementation starts. That would be awesome.

zaki50 commented 7 years ago

No need to have isNull(). Users can do managed1.realmInteger == null like other nullable types.

I would like, also, to take this moment, to mention that it would, absolutely, be possible to have this discussion -- perhaps, even, to bring it to some kind of conclusion -- before implementation starts.

I apologize about this. It's my fault.

bmeike commented 7 years ago

No need to have isNull(). Users can do managed1.realmInteger == null like other nullable types.

I do believe you are correct! Boy, are people going to be surprised the first time they try any of this stuff outside the context of a RealmObject!!!

I apologize about this. It's my fault.

Definitely not your fault. It is systemic. I could have insisted on clarity, before I got started.

nirinchev commented 7 years ago

Does the Java binding have a compile time way to warn people not to use realmInteger outside the context of a RealmObject? E.g. detect that there's a local variable of RealmInteger type and emit a warning/error?

bmeike commented 7 years ago

Yeah, we could probably do that. This just gets weirder and weirder.

bmeike commented 7 years ago

Look, this is just a horrible precedent. We are gonna build something that behaves in such a completely non-Java way, that we are actually talking about using magic to warn them against using it normally. I thought that with our byte-code re-writing powers came great responsibility! I thought we were using them for good! ;-P

zaki50 commented 7 years ago

I do believe you are correct! Boy, are people going to be surprised the first time they try any of this stuff outside the context of a RealmObject!!!

What do you mean? I don't see any surprising result.

RealmInteger oldManaged1Value = managed1.realmInteger;
oldManaged1Value.isValid() // true

realm.beginTransaction();
managed1.realmInteger = null;
realm.commitTransaction();

managed1.realmInteger == null // true
oldManaged1Value == null // false of course
oldManaged1Value.isValid() // false since the counter is disposed
oldManaged1Value.longValue();// throws IllegalStateException
nirinchev commented 7 years ago

You know what they say, the path to hell is lined with good intentions ;) At this point I'm lost as to what the most Java-friendly implementation would be 😕

austinzheng commented 7 years ago

Yeah, this is a horrendously tricky design problem.

The alternative is to have a method on RealmObjects directly that allows you to increment a property, but I don't think any of our supported languages (except maybe C#) allow you to reify the concept of "a property on this object", which means it would have to be stringly typed 😭 .

Zhuinden commented 7 years ago

What does it mean for a realmInteger to be null?

While I realize that RealmInteger is this funky number that you can increment, decrement, and set value to, and if you don't do set then the inc/dec operations get merged together instead of overwriting each other, right? And it seems you can set the value in the RealmInteger be null although I assume calling increment/decrement on a RealmInteger which has field null will crash, so initializing to null in a distributed setting seems like a very powerful way to shoot yourself in the foot during operation merge (or whatever it's called).

Anyways, why not do something with RealmInteger like what you did with @LinkingObjects private final RealmResults?

So, for example

    public class RealmInteger extends RealmObject {
          private Long value;

          public Long get() {
               return value;
          }

          public void set(Long value) {
               this.value = value; // throw in proxy accessor if marked with `@Required`
          }

          public void increment() {
               value += 1; // do your native magic for RealmInt with accessor 
          }

          public void decrement() {
                value -= 1; // do your native magic for RealmInt with accessor
          }
    }

and then you can do

    public class Post extends RealmObject {
         private final RealmInteger likes = /*cannot be null*/ RealmInteger.valueOf(0); // initial value if doesn't exist in Realm

         public RealmInteger getLikes() {
             return likes;
         }
    }

Then the only tricky things you need to consider then is how to handle the set value of RealmInteger in unmanaged object that you insert, but you could ignore the initial value while if it was explicitly set then even from unmanaged object it would be a "set" operation for a managed one, maybe?


Query support would be tricky in the sense that you wouldn't want to do realmInt.value for queries, you want to do realmInt, so the query api would probably need to check against the type and if it's RealmInt then handle it a bit differently, I guess.

bmeike commented 7 years ago

@Zhuinden I'm all over that. Very Java. I'm losing the argument, though. Bring friends! ;-P

bmeike commented 7 years ago

What do you mean? I can't see any surprising result.

I don't think you are looking in the right place!

managed1.realmInteger = null;
managed1.realmInteger == null // true
// some code that may or may not delete this entire row
managed1.realmInteger.isValid() // what does this return? 

The ref is null. It was assigned null, right there. I suspect you are gonna want isValid(), above, available to distinguish between whether the null value for this row's realmInteger has been deleted or not. And, despite that fact that that is an abomination in the Java language, we absolutely can make that work. It looks really weird, though, and we cannot make it work outside the context of a RealmObject. Outside the RealmObject it will NPE.

managed1RealmInteger = null;
managed1RealmInteger == null // true
// doesn't matter what the code here does
managed1.realmInteger.isValid() // NPE!!!!

Here's another example. How about this:

managed1.realmInteger = new RealmInteger(10);
myRealmInteger = managed1.realmInteger;
myRealmInteger = null;
managed1.realmInteger == null // ???

That one is gonna be surprising, dontcha think?

Btw, if these are the semantics, then it actually makes no difference at all whether the RealmInteger object is lazy or always there. You are depending on its being invisible. If it is invisible, I can make it lazy or permanent, without any affect on the API. Currently, my implementation is permanent.

zaki50 commented 7 years ago

If the row is deleted, managed1.realmInteger will throw IllegalStateException. If not deleted, managed1.realmInteger returns null and calling isValid() against it causes NPE.

managed1.realmInteger = new RealmInteger(10);
myRealmInteger = managed1.realmInteger;
myRealmInteger = null;
managed1.realmInteger == null // ???

false since myRealmInteger = null; just cleared the local variable. No effects against persisted data.

By the way, I realized that my proposal is difficult (almost impossible) to implement collectly againt changes from other threads (or other devices).

https://github.com/realm/realm-java/issues/4266#issuecomment-307335626 is the simplest solution??

Zhuinden commented 7 years ago

Is there really a point to being able to do new RealmInteger instead of only RealmInteger.valueOf() inside a RealmObject?

That's mostly what I wonder about, can a RealmInteger have meaning outside of a RealmObject (just like how linkingObjects realmResults only exists in realm models as well)?

Then again I'm a bit sleepy so maybe I'm not taking something important into consideration.

bmeike commented 7 years ago

@zaki50 : got it. What about this:

managed1.realmInteger = new RealmInteger(10);
myRealmInteger = managed1.realmInteger;
// delete the row...
myRealmInteger.isNull();

IllegalStateException?

bmeike commented 7 years ago

@Zhuinden My implementation already depends on the fact that you can't use new on a RealmInteger. You need to use the factory, because I return an instance of either a managed or an unmanaged subclass.

... and I totally agree: yet one more way of stating the problem is that these suckers don't mean much outside the context of the RealmObject. The original functional requirements, though, were that you be able to hold a reference to a live, updating object.

zaki50 commented 7 years ago

@bmeike I think that https://github.com/realm/realm-java/issues/4266#issuecomment-307348997 and following 3 comments described it.

bmeike commented 7 years ago

@zaki50 Did you want to eliminate isValid for RealmIntegers all together and always use exceptions (or, certainly, isValid on the containing instance) to indicate that a row has been deleted or are you advocating this only in the case of a deleted, null-valued integer? ... and I guess I'm still missing how #4266 (comment) handles calling isValid on a null reference.

nirinchev commented 7 years ago

The original functional requirements, though, were that you be able to hold a reference to a live, updating object

That's a surprise to me. Did we agree on that globally (i.e. all bindings), or was it a requirement just for the Java implementation? (and yes, Github badly needs threading...)

bmeike commented 7 years ago

@nirinchev Well, as usual, the "functional requirements" were occasional comments in Slack. There was definitely common sentiment that, although you could not listen for changes on the RealmInteger object, you could hold a reference to an (row X column) object, outside the context of the container (row) object, and that that object would be live. I suspect that this is only the case for Java. In other bindings it is going to be much easier to hide the existence of the RealmInteger object and, thus, the issue of holding one, outside the context of the container, is simply irrelevant.

bmeike commented 7 years ago

@nirinchev has a point, btw. If this is illegal:

RealmInteger myRealmInteger = managed1.realmInteger;

... and, instead, this is legal:

int i = managed1.realmInteger;

Then we are a huuuuuge step closer to making this all work.

It is still magic of a rather greyish variety but I think that, if we give up on ever holding references to these things, then the very last of my objections to @zaki50 's proposals go away. I also think it gets Christian and Alexander where they want to be.

... and I feel like an idiot for not seeing this sooner. If this is what you all have been trying to tell me, I beg your pardon.

Zhuinden commented 7 years ago

Actually what I was wrong about is RealmInteger extends RealmObject, because it is not an object that can be deleted on its own. It is a ManagableObject, so it's great that it was factored out to make this work :smile: but you're already on that.

I think what primarily causes confusion is that it's technically just an "Integer" in the core, but it is a "Counter" from the outside. But as it is a counter, it really shouldn't be just a number.

I think realmObj1.counter = realmObj2.counter has too much implicit meaning and should be forbidden, the only thing that matters is the operators get, set, increment and decrement.

But if reassignment is forbidden, then it works very similarly to @LinkingObjects.


@beeender 's RealmObject.incrementCounter seems to be super-similar to https://github.com/realm/realm-object-store/issues/357#issuecomment-275555967 in concept, but is it really necessary?


Should setting a RealmInteger to null truly mean reset? What does it mean for a counter to be nullable?

As @bmeike said, ability to set RealmCounter to null makes calling isValid() possible to throw NPE; which is never the case for managed RealmLists (although possible for unmanaged ones).

But what I wonder about is this comment https://github.com/realm/realm-object-store/issues/357#issuecomment-277663036 which says,

if we want a Counter type to function as a robust counter which you increment/decrement and occasionally reset then we want to make accidental reset hard.

In which case implicit resets are a bit scary on assignment between RealmInt and RealmInt - as it is not an explicit intention to "reset".

I really do wonder that it should be a final field that exposes the operators and that's that. I might have written that exact same thing at the top of this post though, but now with reference to object-store comment.

Anyways, maybe @cmelchior will know.

bmeike commented 7 years ago

Ok, ladies and gentlemen. I propose the following. It matches nearly all of the requirements I've heard so far:

RealmInteger x = c.x; // compile time error: type mismatch c.x is Long.  Not absolutely certain that I can arrange this...
x = new RealmInteger(); // compile time error: RealmInteger is abstract
x = new RealmInteger() { }; // compile time error: RealmInteger has no public constructor

public class C {
   // blah blah...
   public RealmInteger x;

   public Long getX() { return x; }
   public void setX(Long x) { this.x = x; }
}

realm.beginTransaction();
C c = realm.createObject(C.class);
c.x = 7; 
realm.commitTransaction();

c.x == 7; // true I think
c.x.intValue() == 7; // definitely true

c.x = -1; // throws: not in a transaction
realm.beginTransaction();
c.x = -1;
c.x == -1; // true
c.x.set(4); 
c.x.increment(5);
c.x.decrement(2);
C c2 = realm.createObject(C.class);
c2.x = c.x;
realm.commitTransaction();

c.x == 7; // true
c2.x == 7; // true
c.x == c2.x; // true, I think.
c.x.equals(c2.x); // definitely true

realm.beginTransaction();
c2.x.increment(1);
realm.commitTransaction();
c.x == 7; // true
c2.x == 8; // true

c.x.isValid(); // == c.isValid().  I suggest getting rid of this
c.x.isManaged(); // == c.isManaged().  I suggest getting rid of this

c.x.isNull(); // false.  With @zaki50 : suggest getting rid of this
c.x = null;
c.x == null; // true;
c.x.longValue(); // NPE

C uc = realm.copyFromRealm(c);
// the value of uc.x is now frozen: it will never be updated
// uc behaves exactly as c above, except:
uc.x.isValid(); // true, except NPE if x is null: let's get rid of it.
uc.x.isManaged(); // false, except NPE if x is null: let's get rid of it.

I might have missed something and there are a couple of things that I'm not completely certain that I can pull off.

zaki50 commented 7 years ago

c.x = 7; seems to be assigning int to RealmInteger field. Is there any way to successfully compile it?

bmeike commented 7 years ago

Yeah! I think there is! You totally inspired me! I'm not absolutely positive, but I believe I can do it.

Won't be an int. Will be a Long. You get your nullability!! It depends on exactly how the pre-processor is run, and what kinds of casting java is willing to do...

I gotta try it out... but I want to know if people like it, before I spend another week or so...

bmeike commented 7 years ago

Ok, one more time. Running this up the flag-pole. Who's gonna salute?


    // All asserts are true unless otherwise noted.
    public static void unmanaged() {
        C c1 = new C();
        C c2 = new C();

        c1.x = null; // compiler error: c1.x is final
        c1.x = RealmInteger.valueOf(7); // compiler error: c1.x is final

        c1.x.set(7); // !!! NPE possible
        c2.x.set(Long.valueOf(7)); // !!! NPE possible
        assert c1.x.equals(c2.x);
        assert c1.x != c2.x;

        RealmInteger r1 = c1.x;
        r1.increment(1);
        assert r1.equals(c1.x);
        assert r1 == c1.x;
        assert c1.x.get() == 8;  // !!! undefined
        assert c1.x.get().equals(8L);
        assert !c1.x.equals(c2.x.get());
        assert c1.x.get().intValue() == 8;

        Long n = c1.x.get();
        assert n.equals(Long.valueOf(8));
        assert n.equals(c1.x.get());
        assert n == Long.valueOf(8); // !!! undefined
        assert n == c1.x.get(); // !!! undefined
        assert n.intValue() == c1.x.get().intValue();

        c1.x.increment(1);
        assert n.intValue() != c1.x.get().intValue();
        assert n.intValue() != r1.get().intValue();

        assert !c1.x.isNull();
        c1.x.set(null);
        assert c1.x != null;
        assert c1.x.isNull();
        assert r1.isNull();
        assert r1.get() == null;

        assert r1.isValid();
        assert !r1.isManaged();
    }

    // Exactly as unmanaged except that changing the RI's value requires a transaction.
    public static void managed() {
        Realm realm = Realm.getDefaultInstance();
        realm.beginTransaction();
        C c1 = realm.createObject(C.class);
        C c2 = realm.createObject(C.class);
        c1.x.set(7); // !!! NPE possible
        c2.x.set(7); // !!! NPE possible
        realm.commitTransaction();

        c1.x.increment(1); // !!! throws IllegalStateException

        RealmInteger r1 = c1.x;
        r1.increment(1); // !!! throws IllegalStateException
        realm.beginTransaction();
        r1.increment(1);
        realm.commitTransaction();

        assert r1.isValid() == c1.isValid();
        assert r1.isManaged();
    }

    public static void mixed() {
        Realm realm = Realm.getDefaultInstance();
        realm.beginTransaction();
        C c1 = realm.createObject(C.class);
        c1.x.set(7); // !!! NPE possible
        realm.commitTransaction();

        C c2 = new C();
        c2.x.set(7); // !!! NPE possible
        assert c2.x.equals(c1.x);
        assert c2.x.get().equals(c1.x.get());
        assert !c2.x.equals(c1.x.get());

        RealmInteger r2 = c1.x;
        C c3 = realm.copyFromRealm(c1);
        assert r2 != c3.x;
        assert r2.get().equals(c3.x.get());
        assert r2.get().longValue() == c3.x.get().longValue();

        r2 = c2.x;
        realm.beginTransaction();
        c3 = realm.copyToRealm(c12);
        realm.commitTransaction();
        assert c1.x != c3.x;
        assert c1.x.get().equals(c3.x.get());
        assert r2.get().longValue() == c3.x.get().longValue();
    }

Are there any cases you care about that are not addressed here?

cmelchior commented 7 years ago

Looks fine for the most parts. I have a few concerns/comments about null handling:

I don't understand:

C c1 = new C();
c1.x.set(7); // !!! NPE possible
c2.x.set(Long.valueOf(7)); // !!! NPE possible

Unless you mean it is because people did

public class C extends RealmObject {
  public final MutableRealmInteger x = null; // Allowed, but would be weird.
}

I'm not sure I agree with this:

     C c1 = realm.createObject(C.class);
        C c2 = realm.createObject(C.class);
        c1.x.set(7); // !!! NPE possible
        c2.x.set(7); // !!! NPE possible

This would mean that if you set the value to null, you can never change it again. IMO the managed object should always return a RealmInteger.

realm.beginTransaction();
C c1 = realm.createObject(C.class);
c.x.set(null); // Should always work
c.x.set(42); // Should always work

Also we need to consider how the @Required annotation should work. Right now you can e.g. do @Required String value = "", which will cause Realm to throw an exception if you ever try to assign null to value. We should IMO do the same for MutableRealmInteger, so:

public class C extends RealmObject {
    @Required
     public final MutableRealmInteger val = MutableRealmInteger.valueOf(null); // Not allowed, should throw when calling `realm.createObject()`. Doubt we can catch it before.
}

public class C1 extends RealmObject {
    @Required
     public final MutableRealmInteger val = MutableRealmInteger.valueOf(0);
}

realm.beginTransaction();
realm.createObject(C1.class).val.set(null); // Throw IllegalArgumentException
realm.commitTransaction();
bmeike commented 7 years ago

Exactly: We will not prevent:

    public final MutableRealmInteger x = null; // Allowed, but would be weird.

In fact, we require that, for @LinkingObjects, so it might happen.

You say:

This would mean that if you set the value to null, you can never change it again. IMO the managed object should always return a RealmInteger.

... but you can't set x. It is final. I take your point, though, that the managed object should always have a non-null counter. That totally seems correct.

I also think that you are right about @Required. If normal RealmIntegers are nullable, @Required ones should not be.

Making it so.

Zhuinden commented 7 years ago

I think the problem with the magic trick of making RealmInteger = null would be the inability to provide an initial value.

bmeike commented 7 years ago

This is @cmelchior's original heading for this Issue. It is now quite out of date. I do not want to lose either it or the rest of this history of this issue, so I am moving the original comment here and revising the head of the ticket to reflect current reality:

==================

Core supports CRDT Counter functionality, which is very useful for Sync. As discussed here: https://github.com/realm/realm-object-store/issues/357

Conclusion so far: It is just a wrapper type around Cores INTEGER that also expose any specialized operations there. Currently only "add_int()", but we might allow exposing the int as a bit pattern in the future as well.

Pr. the discussion in the Object Store, the plan is that it should be possible to use RealmInteger interchangeably with standard integers, i.e. changing a field from Integer to RealmInteger should not trigger a migration.

Possible implementation:

// RealmInteger or RealmInt? 
public class RealmInteger extends Number implements Comparable {

  // Constructors
  public RealmCounter(String val);
  public RealmCounter(long val);
  // Possible others?

  // Realm specific methods 
  public void increment(long val); // add() instead? 
  public void decrement(long val); // Need this? or just use increment(-val)?
  public void set(long val); 

 // Standard methods from Integer
 // Which ones to include here? Probably just all of them 
 // https://docs.oracle.com/javase/7/docs/api/java/lang/Integer.html

  // Convert to java primitive types. From Number class
  public int intValue();
  public long longValue();
  public short shortValue();
  public float floatValue();
  public double doubleValue();

  // Comparable
  public int compareTo(RealmInteger other);

  String toString();
  int hashCode();
  boolean equals(Object o);
}

Since it will be a wrapper class I would be inclined to treat it as implicit nullable just like an Integer.

TODO:

Zhuinden commented 7 years ago

Support RealmList

Isn't RealmInteger just a fancy int? I would think RealmList<RealmInteger> is unsupported because it's a primitive.

bmeike commented 7 years ago

@Zhuinden : https://github.com/realm/realm-java/issues/575

Zhuinden commented 7 years ago

@bmeike wooooooooooooooooooooow!!!!!!!

bmeike commented 7 years ago

heh. I'll take that as approval. ;-P

bmeike commented 7 years ago

Filed #5024 for Dynamic Realm support

bmeike commented 7 years ago

Filed #5025 for RealmList support

bmeike commented 7 years ago

Closing. #5024 and #5025 track remaining work