objectbox-java icon indicating copy to clipboard operation
objectbox-java copied to clipboard

Adding non-null Kotlin property causes crash when getting existing entity

Open LeeKingHung opened this issue 6 years ago • 15 comments

Edit: The original issue described below about default values has moved to #157.

This issue is now about correctly detecting not-nullable properties of Kotlin data classes.

Issue Basics

  • ObjectBox version (are using the latest version?): 2.2

I added a new int (not Integer) property into an existing entity. And I found that the new property values for existing record are all null instead of default value (0 in this case), and the the equal query will always return empty result.

box.query().equal( intProperty, 0) -> zero result box.query().notEqual( intProperty, 0) -> zero result box.query().isNull( intProperty ) -> non-empty result

Is this considered a bug? Or I should expect null for primitive property

LeeKingHung avatar Oct 19 '18 07:10 LeeKingHung

Thanks for reporting!

Internally, added properties are by default null. That should be correct.

ping @greenrobot Though not sure if the database should treat null as if the default value was set (0, false, etc.) for primitive types?

-ut

greenrobot-team avatar Oct 29 '18 09:10 greenrobot-team

the same in our code.

We added a field

@Entity
data class Notes(...
var aaa : String = "", //(kotlin)
...)

And the field is null (and it makes no sense)

jrcacd avatar Nov 08 '18 13:11 jrcacd

Interesting that we got a crash in case when we add new field to entity with not nullable type. E.g.

  1. Create entity with one field.
  2. Run the app. Put new entity in db.
  3. Add new field to entity e.g. var someField: Int = 0.
  4. Run the app and get all saved entities from db. In our case it crashes. Is it means that automatic migration is totally broken when we use kotlin? @greenrobot-team @greenrobot Should I create standalone ticket for this case with an example of project where bug can be reproduced?

Yazon2006 avatar Feb 25 '19 16:02 Yazon2006

@Yazon2006 Just tried this with a very simple example and val someField: Int = 0 works, but obviously val anotherField: String = "default" does not.

As a rule of thumb: when adding new properties make them nullable (so String? instead of String). Maybe someone can also come up with some constructor magic (probably only for regular classes).

// before
@Entity
data class Example(
        @Id var id: Long = 0
)
// after
@Entity
data class Example(
        @Id var id: Long = 0,
        val someProperty: Int = 42, // primitive type, maps to 0 for existing entities
        val anotherProperty: String? = "default" // nullable for compat with previous model
)

The issue is that for properties stored as primitives (internally we call this scalars, like long, double, Date) in ObjectBox a null entry is mapped to a non-nullable type when getting an object, e.g. 0 for int. But if the property is stored as a nullable type, like String, ObjectBox will map no entry to null instead when getting the object.

greenrobot-team avatar Feb 26 '19 08:02 greenrobot-team

Yes it's pretty reasonable, I understand. But it can be a bit painful and confusing. E.g. when Entity implements some interface with non null fields we still need to use non null values. And for such case I used workaround like this:

interface SyncItem {
	var itemId: String
	var isSynchronized: Boolean
}

class Entity(
		//ugly duplicated field
		var dbItemId: String? = null,
		override var isSynchronized: Boolean = false
) : SyncItem {

	override var itemId: String
		get() = dbItemId ?: generateItemId()
		set(value) {
			dbItemId = value
		}
}

I would be glad to know if there are other more convenient ways to handle such kind of problems.

Yazon2006 avatar Feb 26 '19 08:02 Yazon2006

We could probably fix this by trying to detect non-null Kotlin properties. Then flag them as such so ObjectBox will not try to init them with null.

The byte-code produced by Kotlin annotates non-null properties with org/jetbrains/annotations/NotNull. Based on a quick check the ObjectBox processor can see it. -ut

greenrobot-team avatar Feb 26 '19 10:02 greenrobot-team

Sounds awesome! Thank you for your efforts! Looking forward.

Yazon2006 avatar Feb 26 '19 10:02 Yazon2006

There are a ton of other artifacts providing these kind of annotations. Here is the relevant ones from the list IntelliJ IDEA covers by default, with Maven coordinates for the artifacts containing them just for the reference:

  • org.jetbrains.annotations.NotNull (org.jetbrains:annotations:17.0.0)
  • javax.annotation.Nonnull (e.g. com.google.code.findbugs:jsr305:3.0.2)
  • edu.umd.cs.findbugs.annotations.NonNull (com.google.code.findbugs:findbugs-annotations:3.0.1)
  • android.support.annotation.NonNull (com.android.support:support-annotations:28.0.0 from https://maven.google.com)
  • androidx.annotation.NonNull (androidx.annotation:annotation:1.0.2 from https://maven.google.com)
  • org.checkerframework.checker.nullness.qual.NonNull (org.checkerframework:checker-qual:2.6.0)
  • com.android.annotations.NonNull (part of Android SDK)

I don't say every one of them should be supported, but it might be reasonable to cover at least javax.annotation and maybe Android ones in addition to Kotlin / JetBrains ones, if Java people find this convenient.

r4zzz4k avatar Feb 27 '19 13:02 r4zzz4k

OK, sorry, I completely let multiple issues get mangled together here.

@LeeKingHung @jrcacd and others, refer to #157 for the issue that default values are not as expected.

This issue is now about correctly detecting not-nullable properties of Kotlin data classes.

@r4zzz4k Thanks for adding this. For now with Kotlin it is more pressing as it will actually lead to a crash due to Kotlin's runtime nullability checks.

greenrobot-team avatar Mar 04 '19 10:03 greenrobot-team

@greenrobot-team Hi, greenrobot team, any update for this issue?

chaxiu avatar Oct 14 '19 16:10 chaxiu

@chaxiu No. But it's easy to avoid by making sure to use nullable properties. See my comment: https://github.com/objectbox/objectbox-java/issues/589#issuecomment-467339129

greenrobot-team avatar Oct 15 '19 06:10 greenrobot-team

I used an annotation that seemed to solve the problem, at least I did. The object to add a field is called Apple, use @BaseEntity to declare an abstract class Fruit, move the new field in Apple to Fruit, and make Apple extend Fruit to solve this problem. I hope to help you.

StealFeam avatar Jan 15 '20 05:01 StealFeam

An alternative workaround to keep a non-null Kotlin property: use @Convert. An example using String:

@Convert(dbType = String::class, converter = NullToEmptyStringConverter::class)
var nonNull: String = ""

class NullToEmptyStringConverter : PropertyConverter<String, String?> {
    override fun convertToDatabaseValue(entityProperty: String): String? {
        return entityProperty
    }

    override fun convertToEntityProperty(databaseValue: String?): String {
        return databaseValue ?: ""
    }
}

greenrobot-team avatar Apr 27 '20 08:04 greenrobot-team

We just published 2.6.0-RC which should not crash.

Additionally, you can now use @DefaultValue("") to introduce a new non-optional string property.

We'd love some feedback on this before releasing! E.g. @DefaultValue may suggest "too much", another name might be @AbsentValue.

greenrobot avatar Apr 28 '20 16:04 greenrobot

Actually adding a non-null property still crashes, but with 2.6.0-RC you will now see a Java exception with the actual reason.

Also, with 2.6.0-RC, instead of manually adding a converter as suggested above, you can now just annotate a non-null String property with @DefaultValue(""):

@DefaultValue("")
var nonNull: String = ""

Keeping this open to as mentioned add support for adding non-null Kotlin properties without user intervention.

greenrobot-team avatar May 04 '20 06:05 greenrobot-team