Array of Primitives: RealmObjectSchema support by cmelchior · Pull Request #5329 · realm/realm-java

cmelchior · 2017-09-26T22:03:41Z

This adds support for primitive lists to `RealmObjectSchema

Going around Object Store and directly to Core to dynamically add primitive lists during migrations. Not ideal, but using Object Store constants where possible.
Refactored Table.cpp#nativeConvertColumnToNotNullable and Table.cpp#nativeConvertColumnToNullable` so they work on both normal tables and sub tables. This is the biggest change, so please focus review there.
Added bunch of tests.

Open questions:
Is RealmObjectSchema.addRealmListField(String name, Class<?> class) the best name? We already have RealmObjectSchema.addRealmListField(String name, RealmObjectSchema class), but is the overload weird? Another alternative could be addPrimitiveRealmListField(name, class) or addPrimitiveListField(name, class)? For the dynamic API, we have getList(string fieldName). This will either need an overload with getList(String fieldName, Class<?> or getPrimitiveList(String fieldName, Class<?> clazz).

Since these lists behavior is different from lists with Realm models, I would be tempted to go with:

`RealmObjectSchema.addPrimitiveListField(String fieldName, Class<?> elementType);
`DynamicRealmObject.getPrimitiveList(String fieldName, Class<?> elementType);

or

`RealmObjectSchema.addValueListField(String fieldName, Class<?> elementType);
`DynamicRealmObject.getValueList(String fieldName, Class<?> elementType);

ValueList is probably more technically correct as we also support more types than just the primitives, even though Date technically isn't a value type. On the other hand, I suspect that primitive type is more familiar to people than value type,

So 3 proposals:

Overload methods:

RealmObjectSchema.addRealmListField(String fieldName, Class<?> elementType);
RealmObjectSchema.addRealmListField(String fieldName, RealmObjectSchema objectSchema);
RealmList<E> list = DynamicRealmObject.getList(String fieldName, Class<?> elementType);
RealmList<DynamicRealmObject> list = DynamicRealmObject.getList(String fieldName);

Use primitive:

RealmObjectSchema.addPrimitiveListField(String fieldName, Class<?> elementType);
RealmObjectSchema.addRealmListField(String fieldName, RealmObjectSchema objectSchema);
RealmList<E> list = DynamicRealmObject.getPrimitiveList(String fieldName, Class<E> elementType);
RealmList<DynamicRealmObject> list = DynamicRealmObject.getList(String fieldName);

Use value:

RealmObjectSchema.addValueListField(String fieldName, Class<?> elementType);
RealmObjectSchema.addRealmListField(String fieldName, RealmObjectSchema objectSchema);
RealmList<E> list = DynamicRealmObject.getValueList(String fieldName, Class<E> elementType);
RealmList<DynamicRealmObject> list = DynamicRealmObject.getList(String fieldName);

Thoughts?

TODO

Fix currently broken unit tests (changing nullability doesn't seem to take effect).
Update IndexFieldType with primitive list types
Update InvalidIndexFieldType with primitive list types
Update InvalidPrimaryKeyFieldType with primitive list types

…hema

…notation_processor

…tive_list_9_RealmList

…ive list

…notation_processor

…tive_list_9_RealmList

…hema

…notation_processor

…tive_list_9_RealmList

- JavaAccessorContext. - Accessors for OsList. String and binary don't work yet. - Tests for OsList.

…tive_list_9_RealmList

# Conflicts: # CHANGELOG.md

Zhuinden · 2017-10-02T13:01:36Z

+     * @throws IllegalArgumentException if the field name is illegal, a field with that name already exists or
+     * the element type isn't supported.
+     * @throws UnsupportedOperationException if this {@link RealmObjectSchema} is immutable.
+     */


I think naming elementType to be primitiveType would make this method easier to understand, that way you don't need to read the javadoc to know that this is for array of primitives only.

Zhuinden · 2017-10-02T13:09:40Z

+        FieldMetaData metadata = SUPPORTED_SIMPLE_FIELDS.get(primitiveType);
        if (metadata == null) {
-            if (elementType.equals(RealmObjectSchema.class)) {
+            if (primitiveType.equals(RealmObjectSchema.class)) {


I wonder if this should check against if (primitiveType.equals(RealmObjectSchema.class) || RealmModel.class.isAssignableFrom(primitiveType)) { to ensure that they get this error message if they provide the Class<?> that belongs to a RealmModel.

Currently I think they'd get RealmList does not support lists with this type: "SomeRealmObject" which could be confusing.

@cmelchior please verify this

Yes, this would be nice. Fixing

nhachicha · 2017-10-01T14:48:07Z

+                    // All simple list types
+                    schema.addRealmListField(fieldName, fieldType.getType());
+                    if (fieldType.isNullable()) {
+                        schema.setRequired(fieldName, true);


are you changing the field from non-required to required?

Yes, changing name of method to increase clarity

nhachicha · 2017-10-02T12:16:34Z

                    } catch (IllegalArgumentException ignored) {
                    }
                    break;
+                default:


does this include all PRIMITIVE_* type as well? i.e setting primitive type to null? Shouldn't this throw?
arent' we missing if (!type.isNullable()) { continue; } ?

I modified the enum, so it only contains non_list types. The lists are check in the next block

nhachicha · 2017-10-02T12:18:55Z

                    break;
            }
        }
+        for (FieldListType fieldType : FieldListType.values()) {


there's no logic tested here? I wonder why you're keeping this block?

A test was being added here in #5351 . I would prefer to keep this for now to decrease merge conflicts.

nhachicha · 2017-10-02T12:56:19Z

+        return table->get_subdescriptor(col)->add_column(data_type, ObjectStore::ArrayColumnName, nullptr, is_column_nullable);
+    }
+    CATCH_STD()
+    return 0;


reinterpret_cast<jlong>(nullptr)

Zhuinden · 2017-10-02T13:13:26Z

     * }
     * </pre>
-     * If the list contains references to other Realm classes, then use
+     * If the list contains references to other Realm classes use


The comma was needed here 😛

Zhuinden · 2017-10-02T19:54:52Z

#5329 (comment)

Zhuinden · 2017-10-03T09:08:47Z

-    public <T> RealmList<T> getValueList(String fieldName, Class<T> valueClass) {
-        // TODO implement this
-        return null;
+    public <E> RealmList<E> getList(String fieldName, Class<E> elementType) {


Sooo, elementType vs primitiveType vs valueType?

primitiveType.
I'm renaming variables right now 😛

Zhuinden · 2017-10-03T09:12:11Z

+        switch (columnType) {
+            case LIST:
+                //noinspection unchecked
+                setModelList(fieldName, (RealmList<DynamicRealmObject>) list);


I think there should be some kind of custom error message for providing a RealmList<? extends RealmModel> that is not RealmList<DynamicRealmObject>.

Currently it'll just throw a ClassCastException

Zhuinden · 2017-10-03T09:12:40Z

-     * @throws IllegalArgumentException if field name doesn't exist, it is not a list field, the type
-     * of the object represented by the DynamicRealmObject doesn't match or any element in the list belongs to a
-     * different Realm.
+     * @param list list of objects. Must either be primitive types or Realm objects.


This should be a bit more specific that it requires {@link DynamicRealmObject}.

…ass type. Add missing unit tests.

Zhuinden · 2017-10-03T10:30:16Z

+            schema.addRealmListField("field", AllJavaTypes.class);
+            fail();
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("Use 'addRealmListField(String name, RealmObjectSchema schema)' instead"));


Zhuinden · 2017-10-03T10:31:33Z

        FieldMetaData metadata = SUPPORTED_SIMPLE_FIELDS.get(primitiveType);
        if (metadata == null) {
-            if (primitiveType.equals(RealmObjectSchema.class)) {
+            if (primitiveType.equals(RealmObjectSchema.class) || RealmModel.class.isAssignableFrom(primitiveType)) {


Zhuinden · 2017-10-03T10:36:56Z

+                // to throw a better error message.
+                // Primitive types are checked inside `setModelList`
+                if (!list.isEmpty()) {
+                    E element = list.first();


I think just catching the possible ClassCastException and re-throwing it with the custom message would be more reliable and less hacky.

I like the message though!

The classcast exception would not catch the generic parameter, only if RealmList was of the wrong type. Type erasure is a pain :(

I see, I guess it'll do. Thank you! 🙂

zaki50 and others added 30 commits September 6, 2017 22:09

Merge branch 'master-4.0' into my/primitive_list_6_OsList

501d6b2

Merge branch 'my/primitive_list_6_OsList' into my/primitive_list_7_Sc…

80c963e

…hema

Merge branch 'my/primitive_list_7_Schema' into my/primitive_list_8_an…

d4333f1

…notation_processor

Merge branch 'my/primitive_list_8_annotation_processor' into my/primi…

b305367

…tive_list_9_RealmList

throw an exception if query or aggregation feature is used for primit…

c48bee2

…ive list

add realm field types

8b07850

SchemaInfo for primitive list

df0e6a0

Merge branch 'my/primitive_list_7_Schema' into my/primitive_list_8_an…

7c420cc

…notation_processor

fix a bug

ed554e9

Merge branch 'my/primitive_list_7_Schema' into my/primitive_list_8_an…

392a877

…notation_processor

fix a bug

2f38412

Merge branch 'my/primitive_list_7_Schema' into my/primitive_list_8_an…

395fc8b

…notation_processor

fix tests

86bd3d4

Merge branch 'my/primitive_list_8_annotation_processor' into my/primi…

a097d8f

…tive_list_9_RealmList

Merge branch 'master-4.0' into feature/primitive_list

1892469

Merge branch 'feature/primitive_list' into my/primitive_list_6_OsList

8cd8e13

Merge branch 'my/primitive_list_6_OsList' into my/primitive_list_7_Sc…

cdf9585

…hema

Merge branch 'my/primitive_list_7_Schema' into my/primitive_list_8_an…

ba3daaa

…notation_processor

Merge branch 'my/primitive_list_8_annotation_processor' into my/primi…

2b007a4

…tive_list_9_RealmList

address findbugs issues

84aca51

added Javadoc comment and RealmList.toArray()

94b109b

fix bug

04c04c1

Merge branch 'my/primitive_list_8_annotation_processor' into my/primi…

c505b4d

…tive_list_9_RealmList

Draft implementation of accessor context

799ee7e

- JavaAccessorContext. - Accessors for OsList. String and binary don't work yet. - Tests for OsList.

implement Primitive List logic for copy() and insert()

c1d2396

Merge branch 'my/primitive_list_8_annotation_processor' into my/primi…

9654a68

…tive_list_9_RealmList

implement Primitive List logic for insert(Realm, Iterator,...)

5243cd6

Merge branch 'my/primitive_list_8_annotation_processor' into my/primi…

36c23ec

…tive_list_9_RealmList

generate code for primitive list in update(), insertOrUpdate()

7cc7ae5

Merge branch 'my/primitive_list_8_annotation_processor' into my/primi…

8fb4430

…tive_list_9_RealmList

More cleanup

787408e

cmelchior changed the base branch from feature/primitive_list to master October 1, 2017 10:18

cmelchior added 4 commits October 1, 2017 12:19

Merge branch 'master' into cm/cm/primitivelist-schema-support

deb9dde

# Conflicts: # CHANGELOG.md

Fix wrong var name

a320044

Merge branch 'master' into cm/cm/primitivelist-schema-support

97d35b5

Remove types no longer visible

686716b

beeender approved these changes Oct 2, 2017

View reviewed changes

Zhuinden reviewed Oct 2, 2017

View reviewed changes

cmelchior added 2 commits October 2, 2017 15:04

Change class type parameter for better readability

45bf86a

Better javadoc

a25e747

Zhuinden reviewed Oct 2, 2017

View reviewed changes

nhachicha reviewed Oct 2, 2017

View reviewed changes

Zhuinden reviewed Oct 2, 2017

View reviewed changes

cmelchior added 3 commits October 2, 2017 16:37

PR feedback

5a02165

Ups

a479f31

Fix unit test

14d0679

nhachicha approved these changes Oct 2, 2017

View reviewed changes

DynamicRealm support for arrays of primitives (#5351)

3fa8e2e

Zhuinden reviewed Oct 3, 2017

View reviewed changes

Rename variables to primitiveType. Better error messages for wrong Cl…

ca9217c

…ass type. Add missing unit tests.

Zhuinden reviewed Oct 3, 2017

View reviewed changes

cmelchior merged commit b922b09 into master Oct 3, 2017

cmelchior deleted the cm/cm/primitivelist-schema-support branch October 3, 2017 10:43

Zhuinden mentioned this pull request Oct 3, 2017

Realm Java 4.0 Doc updates #5321

Closed

github-actions Bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Conversation

cmelchior commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zhuinden Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zhuinden commented Oct 2, 2017

Uh oh!

Zhuinden Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zhuinden Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cmelchior commented Sep 26, 2017 •

edited

Loading

Zhuinden Oct 2, 2017 •

edited

Loading

Zhuinden Oct 3, 2017 •

edited

Loading

Zhuinden Oct 3, 2017 •

edited

Loading