Support for overriding internal names using annotations #5280

cmelchior · 2017-09-19T09:46:36Z

This PR adds support for creating a mapping between internal names used by Realm and those used in the model classes which e.g. makes it easier to share schemas between languages without compromising the standard naming conventions in Java

it is also possible to apply a RealmNamePolicy for a class or module instead of being forced to manually override each and every field.

The basic implementation is a simple mapping scheme in Java before any relevant field or class name is sent to Object Store. As long as Object Store does not support a true dynamic mode, we cannot have this functionality living there.

// Set defaults on the module affecting all classes
@RealmModule(classNamingPolicy = RealmNamePolicy.CAMEL_CASE, fieldNamingPolicy = RealmNamePolicy.CAMEL_CASE, allClasses = true)
public MyModule {
}

// Set class specific overrides
@RealmClass(name = "!Person", fieldNamingPolicy = RealmNamePolicy.PASCAL_CASE)
public class Person extends RealmObject {

    // Field specific overrrides
    @RealmField(name = "internal_name")
    public string name;

}

Especially, the following semantics are in place:

Queries on Realm (typed Java model classes) still uses the names defined in Java
Queries on DynamicRealm uses the internal names.
Migrations and RealmObjectSchema uses the internal names. This might seem really annoying, but the primary use case is sync, where migrations are automatic.
This does not have any impact on importing from JSON which must still use the Java names. As discussed in @SerializedName annotation for defining Class/Field names #1470 this is two different concerns. It would be relatively straightforward to add a useInternalNameForJson to these annotations though Schema errors will use the internal names instead of the external ones.
RealmProxyObject toString() still uses Java names

Review Guide:

Biggest refactor is in the annotation processor where modules are now processed in two steps (pre- and post-). See ModuleMetaData. This covers detecting proper use of the introduced naming policies
ColumnInfo was refactored to store both external and internal names. Most refactors here are adding Javadoc + renaming variables to express intent better.
Quite a lot of unit tests added. See RealmNameTests for annotation processor tests that test converter logic + proper use of annotations
See CustomRealmNameTests for library unit tests that mostly test the functionality of queries and schema API's.
Original implementation used Guava for mapping names, but it was not powerful enough, so a custom built solution was added. I used a slightly changed variant of the one used in https://github.com/cmelchior/realmfieldnameshelper, so it should be fairly battle-tested.

TODO:

Figure out naming for the annotation and related classes
Determine the number of policies supported and any restrictions.
Implement query support
Implement schema support
Implement DynamicRealm support
Implement detection of conflicting policies for a class part of different modules.
Check that library and app modules do not conflict

…me of classes and fields.

Conflicts: realm/realm-annotations-processor/src/main/java/io/realm/processor/RealmProxyClassGenerator.java realm/realm-library/src/main/java/io/realm/internal/ColumnIndices.java

Zhuinden · 2017-09-19T23:00:22Z

it is also possible to apply a RealmNamePolicy for a class or module instead of being forced to manually override each and every field.

I don't really get the "policy" thing (I know it's like from GSON, but...) I'd just expect something like a @RealmField annotation and if I want to set a different name, then set it.

Maybe that's just me though. I just think the policy is not as essential as the ability of setting a schema field name.

cmelchior · 2017-09-20T08:42:59Z

@Zhuinden The problem with @FieldName is that it doesn't work for classes or modules.

Originally I was playing around with overloading @RealmClass(name = "custom") and @RealmField(name = "custom), but it feels awkward to use two different annotations for the same thing. Also, it left the question of what to do on modules.

Modules are a bit of a special case though and allowing @RealmName on there complicates things quite a lot, so we could also consider not allowing @RealmName there. But that means there is no place to define a naming policy for an entire Realm (as the RealmConfiguration is not available to the annotation processor).

Zhuinden · 2017-09-20T10:05:42Z

@RealmClass(name = "custom") and @RealmField(name = "custom), but it feels awkward to use two different annotations for the same thing.

I think it is the distinction between @Table/@Column or @JsonObject/@JsonField.

cmelchior · 2017-09-20T10:10:27Z

I guess you are right. So there is precedence for that 👍
Having @RealmField would also be nice for other possible future enhancements. I guess we could overload modules the same way:

@RealmModule(policy = RealmNamePolicy.IDENTITY, allClasses = true)
public void MyModule {
}

@RealmClass(name = "custom)
public class Persons extends RealmObject {

  @RealmField(name = "bar)
  public String foo;
}

I kinda like that. It also means we can remove the recursive flag on e.g @RealmModule and @RealmField

Zhuinden · 2017-09-20T11:44:38Z

@cmelchior The only tricky thing though is that you already have a @RealmClass annotation that you inherit from RealmObject!

cmelchior · 2017-09-20T11:46:48Z

Yes, but that last one would take priority I would assume, but I haven't tested how the annotation processor actually behaves in that scenario

cmelchior · 2017-09-20T21:29:43Z

Just for reference. This is how LoganSquare does it: bluelinelabs/LoganSquare@41572aa

# Conflicts: # realm/realm-annotations-processor/src/main/java/io/realm/processor/ClassMetaData.java # realm/realm-annotations-processor/src/main/java/io/realm/processor/RealmProcessor.java # realm/realm-annotations-processor/src/main/java/io/realm/processor/RealmProxyClassGenerator.java

…mModule, RealmClass and a new RealmField annotation.

# Conflicts: # realm-annotations/src/main/java/io/realm/annotations/RealmClass.java # realm/realm-annotations-processor/src/main/java/io/realm/processor/RealmProxyClassGenerator.java

# Conflicts: # realm/realm-annotations-processor/build.gradle # realm/realm-annotations-processor/src/main/java/io/realm/processor/RealmProcessor.java

Zhuinden · 2018-01-19T00:36:52Z

...cessor/src/test/resources/some/test/NamePoliceConflictingModuleDefinitionsForAllClasses.java

 import some.test.AllTypes;

-public class NamingPolicyConflictingModules {
+public class NamePoliceConflictingModuleDefinitionsForAllClasses {


# Conflicts: # CHANGELOG.md # realm/realm-annotations-processor/src/main/java/io/realm/processor/RealmProxyClassGenerator.java

cmelchior · 2018-01-22T11:43:05Z

Ready for review @nhachicha
It still isn't parsing CI, but any changes to fix that should not effect the semantics.

kneth

Reviewing doc; disclaimer: not a native English speaker.

kneth · 2018-01-22T11:49:50Z

realm-annotations/src/main/java/io/realm/annotations/RealmModule.java

+
+    /**
+     * The naming policy applied to all classes part of this module. The default policy is {@link RealmNamingPolicy#NO_POLICY}.
+     * To define a naming policy for all fields in those classes, use {@link #fieldNamingPolicy()}.


"those" -> "the"

kneth · 2018-01-22T11:53:22Z

realm-annotations/src/main/java/io/realm/annotations/RealmNamingPolicy.java

+ * This enum defines the possible ways class and field names can be mapped from what is used in Java
+ * to the name used internally in the Realm file.
+ * <p>
+ * Examples where this can be useful is e.g:


Remove "e.g"
"is" -> "are"

This can't be right!! 😄

kneth · 2018-01-22T12:01:43Z

realm-annotations/src/main/java/io/realm/annotations/RealmNamingPolicy.java

+ *     </li>
+ * </ol>
+ * <p>
+ * Note, that changing the internal name does <i>NOT</i> effect importing data from JSON. The JSON


kneth · 2018-01-22T12:04:10Z

realm-annotations/src/main/java/io/realm/annotations/RealmNamingPolicy.java

+     * if set in {@link RealmClass#fieldNamingPolicy}, the module policy will still apply to field
+     * names.
+     * <p>
+     * If two modules disagree on the policy and one of them is {@code NO_POLICY}, the other one


"other one" -> "other"

kneth · 2018-01-22T12:09:17Z

...ealm-annotations-processor/src/main/java/io/realm/processor/nameconverter/WordTokenizer.java

+ *         Example is "_FooBar" or "_Foo$Bar" which both becomes "Foo" and "Bar".
+ *     </li>
+ *     <li>
+ *         Anytime your switch from a lower case character to a upper case character as


"your" -> "you"

kneth · 2018-01-22T12:09:47Z

...ealm-annotations-processor/src/main/java/io/realm/processor/nameconverter/WordTokenizer.java

+ *     </li>
+ *     <li>
+ *         Anytime your switch from a lower case character to a upper case character as
+ *         identified by a Character.isUpperCase(codepoint)` and `Character.isLowerCase(codepoint)`.


Missing backquote (after "by a")

kneth · 2018-01-22T12:10:22Z

...ealm-annotations-processor/src/main/java/io/realm/processor/nameconverter/WordTokenizer.java

+ *         Examples are "my😁" and "MY😁" which are both treated as one word.
+ *     </li>
+ *     <li>
+ *         Hungarian notation, i.e. Strings starting with lowercase "m" followed by uppercase letter


"Strings" -> "strings"

Zhuinden · 2018-01-22T13:27:34Z

realm-annotations/src/main/java/io/realm/annotations/RealmNamingPolicy.java

 * to the name used internally in the Realm file.
 * <p>
- * Examples where this can be useful is e.g:
+ * Examples where this are useful:


this are? That can't be right, the previous was better 😛

It's probably is.

Zhuinden · 2018-01-22T13:28:41Z

realm-annotations/src/main/java/io/realm/annotations/RealmNamingPolicy.java

 * </ol>
 * <p>
- * Note, that changing the internal name does <i>NOT</i> effect importing data from JSON. The JSON
+ * Note that changing the internal name does <i>NOT</i> effect importing data from JSON. The JSON


Now I wonder if this is affect or effect.

I never know that.

https://www.vocabulary.com/articles/chooseyourwords/affect-effect/

It should be affect I think :)

Zhuinden · 2018-01-22T13:30:13Z

realm-annotations/src/main/java/io/realm/annotations/RealmNamingPolicy.java

     * <p>
-     * If two modules disagree on the policy and one of them is {@code NO_POLICY}, the other one
-     * will be chosen without an error being thrown.
+     * If two modules disagree on the policy and one of them is {@code NO_POLICY}, the other will


maybe the other module's policy

But what happens if there are 3 @RealmModules and they all have conflicting policy? Which one will override NO_POLICY?

For each class you find all modules it is part of, then you compare the policy between all these 3, so it will throw. The implementation is slightly different, but the effect is the same.

nhachicha

Nice PR, some minor comments, also I didn't see tests with models defined using RealmModel interface

nhachicha · 2018-01-23T12:34:04Z

realm/realm-annotations-processor/src/main/java/io/realm/processor/RealmFieldElement.java

@@ -0,0 +1,124 @@
+/*
+ * Copyright 2017 Realm Inc.


nhachicha · 2018-01-23T12:47:40Z

realm/realm-annotations-processor/src/main/java/io/realm/processor/ModuleMetaData.java

+ * <ol>
+ *  <li>
+ *      Pre-processing. Done by calling {@link #preProcess(Set)}, which will do an initial parse
+ *      of the modules and build up all information it can before before processing any model


remove before

nhachicha · 2018-01-23T17:39:10Z

realm/realm-annotations-processor/src/main/java/io/realm/processor/ModuleMetaData.java

+     * Validates that the class/field naming policy for this module is correct.
+     *
+     * @param globalModuleInfo list of all modules with `allClasses` set
+     * @param classSpecificModuleInfo


missing Javadoc

nhachicha · 2018-01-23T17:45:27Z

realm/realm-annotations-processor/src/main/java/io/realm/processor/ModuleMetaData.java

+            }
+
+            // Check for conflicts with specifically named classes. This can happen if another
+            // module are listing specific classes with another policy.


nhachicha · 2018-01-23T17:54:20Z

realm/realm-annotations-processor/src/main/java/io/realm/processor/ModuleMetaData.java

+        // with NO_POLICY, meaning it will not trigger any errors.
+        if (moduleAnnotation.allClasses()) {
+            // Check for conflicts with other modules with `allClasses` set. Only need to
+            // check the first since other conflicts would have been detected in an earlier


doesn't this depend on the order on which they were added?
ex: we added the following modules :

module 1allClass

module 2allClass

module 3 allClass
now we add module4 which is incompatible with module 2, how can you detect this if you're checking only the first one?

Hmm thinking about this, you might be right. E.g. the following sequence of illegal modules would not be detected.

module1: NO_POLICY
module2: PASCAL_CASE
module3: NO_POLICY
module4: CAMEL_CASE

I'll compare against all previous modules.

nhachicha · 2018-01-24T12:16:44Z

realm/realm-annotations-processor/src/test/resources/some/test/NamePolicyModuleDefaults.java

+import io.realm.annotations.RealmNamingPolicy;
+
+/**
+ * Class with only a custom name


where's the custom name definition? or do you mean the module that uses this class use a custom name?

nhachicha · 2018-01-24T12:22:25Z

realm/realm-library/src/androidTest/java/io/realm/CustomRealmNameTests.java

+    public void dynamicQueryWithInternalNames() {
+        // Backlink queries not supported on dynamic queries
+        RealmResults<DynamicRealmObject> results = dynamicRealm.where(ClassWithPolicy.CLASS_NAME)
+                .equalTo(ClassWithPolicy.FIELD_CAMEL_CASE, "foo") // Java name in model class


Java name? shouldn't be internal name for Dynamic Realm. same for below

If the class no longer exists, then you can't obtain any field-specific / class-specific policy configuration for it.

So I wonder if DynamicRealm should be able to see Java name.

DynamicRealms has to work with the assumption that no java model classes exists. That this test case actually references ClassWithPolicy is just so it is easier to maintain the test case.

But you are correct, DynamicRealms do not understand the concept of name policies as that concept isn't relevant to them.

nhachicha · 2018-01-24T12:29:13Z

...m/realm-library/src/androidTest/java/io/realm/entities/realmname/CustomRealmNamesModule.java

@@ -0,0 +1,16 @@
+package io.realm.entities.realmname;


nhachicha · 2018-01-24T12:29:24Z

.../realm-library/src/androidTest/java/io/realm/entities/realmname/DefaultPolicyFromModule.java

@@ -0,0 +1,7 @@
+package io.realm.entities.realmname;


nhachicha · 2018-01-24T12:32:57Z

realm/realm-library/src/main/java/io/realm/internal/ColumnInfo.java

+            buf.append(", InternalFieldNames=[");
+            boolean commaNeeded = false;
+            for (Map.Entry<String, ColumnDetails> entry : indicesFromColumnNames.entrySet()) {
+                if (commaNeeded) { buf.append(","); }


commaNeeded = true; should be inside the if?

No, in that case, it would never be set to the value true.

Zhuinden · 2018-01-24T14:33:09Z

realm/realm-library/src/main/java/io/realm/internal/fields/DynamicFieldDescriptor.java

     *
     * @param table the start table.
-     * @param fieldDescription the field description.
+     * @param fieldDescription the field description using internal column columns.


column columns?

Zhuinden · 2018-01-26T12:18:09Z

...ealm-annotations-processor/src/main/java/io/realm/processor/RealmProxyMediatorGenerator.java

            @Override
            public void emitStatement(int i, JavaWriter writer) throws IOException {
-                writer.emitStatement("return %s.getSimpleClassName()", qualifiedProxyClasses.get(i));
+                writer.emitStatement("return \"%s\"", internalClassNames.get(i));


That does seem more Proguard-friendly.

Not to mention correct 😉 The previous implementation did not work if you renamed the class name using @RealmClass(name = "othername").

cmelchior · 2018-01-26T12:26:13Z

All comments addressed @nhachicha
I added a runtime check that throws if conflicts between internal names are detected among different modules (not possible to check at compile time). It isn't really possible to create a unit test for this, but I verified that we correctly throw an exception using our moduleExample.

nhachicha · 2018-01-26T18:06:38Z

realm/realm-library/src/main/java/io/realm/internal/modules/CompositeMediator.java

                for (Class<? extends RealmModel> realmClass : mediator.getModelClasses()) {
+                    // Verify that the module doesn't contain conflicting definitions for the same
+                    // underlying internal name. Can only happen if we add a module from a library and
+                    // and a module from the app at the same time.


cmelchior added 3 commits September 7, 2017 19:54

Add support for @RealmName which makes it possible to override the na…

d871340

…me of classes and fields.

Merge branch 'master' into cm/realmname-annotation

f6a3eb1

Merge branch 'master' into cm/realmname-annotation

fbccd5d

Conflicts: realm/realm-annotations-processor/src/main/java/io/realm/processor/RealmProxyClassGenerator.java realm/realm-library/src/main/java/io/realm/internal/ColumnIndices.java

realm-ci assigned cmelchior Sep 19, 2017

This was referenced Sep 19, 2017

realm can user annotation replace column name or table name #5278

Closed

Support mapping from field name in model class to column name in database file. #2476

Closed

cmelchior added 16 commits October 4, 2017 15:09

Remove RealmName annotation. Add methods for specifying names to Real…

7020e4c

…mModule, RealmClass and a new RealmField annotation.

Fix compile issues

b4eff2b

Fix Javadoc and most unit tests.

d8540d7

Add test for conflicting modules

a06f98f

Begun work on checking conflicting policies

1452261

Merge branch 'master' into cm/realmname-annotation

5868b24

# Conflicts: # realm-annotations/src/main/java/io/realm/annotations/RealmClass.java # realm/realm-annotations-processor/src/main/java/io/realm/processor/RealmProxyClassGenerator.java

Merge branch 'master' into cm/realmname-annotation

5dda526

# Conflicts: # realm/realm-annotations-processor/build.gradle # realm/realm-annotations-processor/src/main/java/io/realm/processor/RealmProcessor.java

Fix merge

961db7c

Use correct version of Object Store

cbeec9c

Better docs

295acaf

Backup

92d216d

Processor tests are green

0bc1ea1

Fleshing out unit tests for using name policies in annotations

7203d02

Cleanup

6595fdd

Fix required value lists + cleanup

f24085c

Zhuinden reviewed Jan 19, 2018

View reviewed changes

cmelchior added 4 commits January 22, 2018 11:01

Unify build tools

eee5b5a

Cleanup

a0e73d3

Merge branch 'cm/realmname-annotation'

ebd7e58

# Conflicts: # CHANGELOG.md # realm/realm-annotations-processor/src/main/java/io/realm/processor/RealmProxyClassGenerator.java

Remove FIXMEs

f1930f2

cmelchior requested a review from nhachicha January 22, 2018 11:43

kneth reviewed Jan 22, 2018

View reviewed changes

cmelchior added 2 commits January 22, 2018 14:13

Fix tests after merge from master

1829813

PR feedback

84fe874

Zhuinden reviewed Jan 22, 2018

View reviewed changes

More PR feedback

2fe2fd4

nhachicha reviewed Jan 24, 2018

View reviewed changes

Zhuinden reviewed Jan 24, 2018

View reviewed changes

PR feedback

f4d0979

cmelchior mentioned this pull request Jan 25, 2018

Rewrite annotation processor in Kotlin #5712

Closed

cmelchior added 3 commits January 25, 2018 17:20

More PR feedback

3bd6f07

Output correctly internal name

6bb9fc8

Fix library module mediators not being correctly created

68d22e9

Zhuinden reviewed Jan 26, 2018

View reviewed changes

Fix missing spaces in error message

9a36e84

Better example

be7a2a5

nhachicha approved these changes Jan 26, 2018

View reviewed changes

Improve wording

adfce10

cmelchior merged commit f089a87 into master Jan 29, 2018

cmelchior deleted the cm/realmname-annotation branch January 29, 2018 07:47

cmelchior mentioned this pull request Feb 2, 2018

Correctly support multiple classes with the same name #5737

Merged

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

Support for overriding internal names using annotations #5280

Support for overriding internal names using annotations #5280

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Zhuinden commented Sep 20, 2017

Uh oh!

cmelchior commented Sep 20, 2017

Uh oh!

Zhuinden commented Sep 20, 2017

Uh oh!

cmelchior commented Sep 20, 2017

Uh oh!

cmelchior commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmelchior commented Jan 22, 2018

Uh oh!

kneth left a comment

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 Jan 22, 2018 • 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!

nhachicha left a comment

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 Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

cmelchior commented Sep 19, 2017 •

edited

Loading

Zhuinden commented Sep 19, 2017 •

edited

Loading

cmelchior commented Sep 20, 2017 •

edited

Loading

Zhuinden Jan 22, 2018 •

edited

Loading

Zhuinden Jan 24, 2018 •

edited

Loading