Make fields getter/setter private

[MFlisar]

I'm always using the same naming and technique, like following (see the comment to get what I want):

@TableModelSpec(className="GroupedFolder", tableName="groupedFolder")
@Implements(interfaceClasses = {Serializable.class})
public class GroupedFolderEntrySpec
{
    // make the generated function "setInternalType" private, because from outside, only the ModelMethod below should be used
    private int internalDataSource;

    enum Type
    {
        New,
        Default,
        Custom
    }

    @ModelMethod
    public static void setType(GroupedFolder item, Type type) {
        item.setInternalType(type.ordinal());
    }
}

Can I somehow let squidb make a getter/setter private? It does not respect the private definition of my variable...

[sbosley]

Two things to take note of here: 1) SquiDB actually supports enum data types for columns. We serialize them as a string by default, but if all you want is to have an enum column serialized to an int, you could do so by disabling the code generator's default enum handling (using the disableEnumProperties option) and writing a code generation plugin modeled after the default EnumFieldPlugin. Then you could handle enums however you like. 2) If on the other hand you really wanted to make getters/setters private, you could do that with a code generation plugin as well. You could disable the default getters and setters with the disableGettersAndSetters option and then write your own plugin to manually generate getters and setters using some other rule.

Edit: in your example, I think you don't actually want the generated getter/setter to be private -- you want it to be package private, since you're calling it from outside the generated model class in the model method defined in the spec class.

Personally I'd go with option 1 as it would probably be much less code, but both solutions should work.

[sbosley]

Now that I'm thinking about it, it would probably be useful to have the option of whether to serialize enums using their int ordinal or string name be configurable, so that's something we can look into adding. Shouldn't be too hard.

[MFlisar]

We serialize them as a string by default

This means, enums would be saved to the database as a string field with the enums name? Did not know that I can use enums directly. I would really prefer an integer field for an enum though. Having the ability to set this up somehow would be very useful.

Secondly, what does speak against generating the getters/setters with the same visibility as the the one defined in the model spec class?

[sbosley]

Yes, by default we serialize enums to the db using the enum's name. I can totally understand a preference to use the int ordinal, so I'll look into that next week. The main caveat with that is that when using the SQL builders, non-primitive types are bound to the query as strings, so if you used ints you'd have to always remember to build your criterions like this: MyModel.ENUM_FIELD.eq(Enum.SOME_VAL.ordinal()). I'll have to think more deeply about other possible implications of using ints for enums instead of string name before making any changes or enhancements there.

In terms of the model spec class, they're not really meant to be thought of as "real" objects, so by design access modifiers are ignored. The fields are only there to be a shorthand for a column declaration, so in most of the models we write we leave off the access modifier entirely -- which is technically package-private.

Generally, a getter/setter would only need to be private if some other method were intended to be used exclusively like in your example -- say, to convert a higher-level type into a primitive type for persisting to the db -- so in such cases we would typically use a code generation plugin for that as well. You can see examples of this approach in the EnumPropertyGenerator class as well as our JSON plugin. You can see how in these classes, the getTypeForAccessors() method returns the higher level type to use for the getter and setter, and then the writeGetterBody() and writeSetterBody() control converting that higher level type into the more primitive type for data storage.

Does that help answer your question?

[MFlisar]

Thanks for looking into this.

And yes, this answers my question. Still, I don't see the reason for not supporting some faster way to define the visibility of the setter (via an annotation or via "inheriting" it from the model spec class). I though know, how I can solve the problem

[jdkoren]

Enum name versus ordinal is an interesting question. If we use the name (which is what we currently do), you can change the order and add new values in any position without invalidating anything already stored in your database, but you can't change any of the names. If we use the ordinal, you can change the names without invalidating anything already stored, but you can't change the order of any values, including when adding new values or removing ones that are no longer useful. I think we decided that using the names provided better consistency, but we can look into making it configurable.

*Of course, you could always write database upgrade logic to convert old values to new ones. Edit: In fact, now that I think about it, writing such a migration is probably easier to do using the names rather than figuring out the old and new ordinal values.

[MFlisar]

Whenever I use enumeration I go following approach:

• define enums and always use their ordinal to persist or serialise data
• renaming of enums is no problem
• if I reorder them I extend the enum class and add an id field that I map to the ordinal for all already existing enums and from this point on it use the Id instead of the ordinal for persisting. From now on it can rename and reorder the enums without problems and without updating any reference...

Of course I try to do the last step from the beginning if possible if I think I need it (normally always when I persist data...)

[sbosley]

Yeah, I think both enum approaches are valid. The name approach fits more with the use cases we've encountered when developing/using SquiDB, so we will probably stick with it as the default. If possible, we may be able to add configurability to use ordinals. If we decide it's not suitable for the core though, that's exactly why we created the plugin API in the first place -- so that users have a great deal of control to change or reimplement the default handling.

As far as visibility goes, non-public accessors has just never been a use case we've encountered, so it was never really considered. It's also worth noting that generic get/set methods for any property exist on all model classes and the generated accessors simply wrap these generic ones -- so no getter/setter could ever truly be private as there would always be a way around it. (This is a core architectural detail of how model objects work under the hood so is unlikely to change). That said, we can also consider if an annotation or some other approach would be a worthwhile addition to the squidb core, or if it is better left to a user plugin.

[sbosley]

Update on the current status for this issue, at least with regard to the specific use case of enums:

After thinking about it a bit more, I don't think that a configuration option to use ordinal instead of name makes sense for SquiDB core. As we've already discussed, the ordinal breaks down if you ever reorder or add items to the enum, and if you ever switched to the proprietary id based approach you outlined (which is certainly a good approach), then any configuration option to use ordinal would become useless again and we'd be right back where we started.

So, with regard to the question of things like enums, there's a two-faceted solution that I think makes the most sense here. Part 1 is serialization. Code generation plugins are great at controlling how non-primitive types are serialized -- that's one of the reasons we created them, and it applies well here. To control how enums are serialized, creating a custom plugin in the style of the default enum handling plugin is the best solution -- then you can use ordinal if you wish, and easily switch to using a proprietary id if that ever changes.

Part 2 of the solution is controlling how non-primitive values are bound to queries -- we want to provide an API that allows converting, e.g., an enum literal into something other than it's name when it appears in a SquiDB query object. This is necessary because if you did write a custom plugin for serialization, you'd want the clause MyModel.ENUM_FIELD.eq(MyEnum.SOME_VAL) to compile into myModel.enumField = 0, rather than myModel.enumField = 'SOME_VAL' which is what SquiDB would do by default. PR #217 introduces an API to solve this part of the problem.

The issue of access modifiers like public or private for fields/getters/setters is something of a separate issue. PR #218 enhanced the plugin API to allow plugins greater control over the generated getters/setters, including changing the access modifiers. Still considering whether or not control over access modifiers is a good fit for SquiDB core, or is better left to a separate plugin, but it is now at least possible for users to control it themselves.

[MFlisar]

Enums

I understand. I'm anyway using model methods for my enums everywhere, I may define an interface for my enums and use a code generation plugin in the future (have not tried this yet though).

Modifiers

That's just a beautification for me (as I'm using model methods instead of code generation plugin), so it's not that important - this was the only use case I had for until now. Although, writing some helper and management functions in the model may be a use case, where you want that as well

Pull request

EnumProperty.literal now correctly uses name() instead of String.valueOf()

Does this mean, that this change breaks all enums, that are overwriting the toString function? Then I'm glad that I use model methods that do the int <=> enum mapping manually for me...

[sbosley]

Any enums that are overriding toString() are already broken without this PR. They'd be written to the db using toString(), but would throw an exception as soon as they were read back from the db, because the Enum.valueOf() call would fail. If anyone had encountered this problem in the wild I imagine we'd have heard about it. This change fixes things before it has a chance to become a real problem.

[sbosley]

One more follow-up: I just realized that there is much simpler solution to your original question about modifiers, one that uses a plugin but does not require a new annotation or any of the SquiDB changes we've discussed here (so it will work with the current release).

The following plugin will make all getters/setters take on the access modifiers of the field generating them:

public class ModifierAdjustingPlugin extends Plugin {

    public ModifierAdjustingPlugin(ModelSpec<?> modelSpec, PluginEnvironment pluginEnv) {
        super(modelSpec, pluginEnv);
    }

    @Override
    public void beforeEmitGetter(JavaFileWriter writer, PropertyGenerator propertyGenerator,
            MethodDeclarationParameters getterParams) throws IOException {
        adjustMethodDeclarationParameters(propertyGenerator, getterParams);
    }

    @Override
    public void beforeEmitSetter(JavaFileWriter writer, PropertyGenerator propertyGenerator,
            MethodDeclarationParameters setterParams) throws IOException {
        adjustMethodDeclarationParameters(propertyGenerator, setterParams);
    }

    private void adjustMethodDeclarationParameters(PropertyGenerator propertyGenerator, MethodDeclarationParameters params) {
        VariableElement sourceField = propertyGenerator.getField();
        if (sourceField != null) {
            Set<Modifier> modifiers = new HashSet<>(sourceField.getModifiers());

            // This makes things work appropriately for ViewModels, where the fields are static but the getter/setter are not
            modifiers.remove(Modifier.STATIC);
            params.setModifiers(modifiers.toArray(new Modifier[modifiers.size()]));
        }
    }
}

That's it! This will make the getters/setters copy for a field copy the field's access modifiers -- public fields get public accessors, protected fields get protected accessors, and fields with no modifiers get package-protected accessors. All you'd have to do is add the plugin to your build process as described here. We may or may not someday put this feature into SquiDB itself, but this is the easiest way for you to get the specific feature you're asking for today.

[sbosley]

All of the enhancements discussed in this issue have been released in 3.2.0 (out today), so I'm going to close it. Mainly, this includes 1) enhancements to the plugin API to make it easier to modify the method signatures of getters/setters and 2) an API to make it possible to bind non-primitive arguments to SQL statements using custom logic (e.g. use an enum id instead of name when an enum value is found in a Query).

I am still inclined to leave the ability to change access modifiers to a user plugin, rather than making it a configurable option in core. I think in general, when it comes to the code generator, a philosophy of extensibility rather than configurability will help keep things easier to maintain and prevent feature creep, given the vast array of things users might want to tweak about the code generator -- and for plugins or tweaks that prove extremely popular or in high demand we can always consider merging them into core. So for now, I recommend modifying access modifiers using a plugin (either using the standalone method mentioned above, or a custom plugin to handle enum fields as discussed earlier), and if there's a large amount of demand for this in the future, we can revisit making that a configurable thing in the core code generator.