Initialise copied constants after everything else

[lnikkila]

Hello!

I found a slight problem with the generated initialisation order for constants after I tried to reference some properties in them.

I fixed it by modifying the processor plugin to write the constants just before the constructors. A test case is also included.

Thanks for the excellent work with the library!

---

This makes it possible to reference e.g. properties defined in the generated model from constants in the spec. Some useful use cases for such constants include defining a natural order for the model or some commonly used queries so that they can be accessed easily.

If these are defined before the static final class variables they're trying to reference, it will lead to unexpected behaviour since those dependencies will be null.

For instance, the following seems to work fine when defined in a spec and used from a model, but only because Query.select defaults to selecting everything when the parameter is null:

public static final QUERY_ALL = Query.select(Model.PROPERTIES).freeze();

The following, on the other hand, will crash with a NullPointerException since Model.ID hasn't been initialised yet:

public static final DEFAULT_ORDER = Model.ID.asc();

Detailed information about the initialisation order of class variables can be found in the Java Language Specification.

[sbosley]

First off, this looks great! The change and its motivation totally make sense, and thanks too for including some test cases with it -- that makes our lives so much easier. There's a couple things we need to figure out before merging, one related to the code, the other legal.

First, the legal: It's possible you may need to sign a CLA of some kind before we merge this -- I will double-check about that on Monday and let you know what I find out, but I'm confident that we can get this change in soon.

Second: this change makes a lot of sense for table models, but it's not immediately clear to me if it solves the same problem for view models -- generated models for SQL views refer to their model spec classes in the schema definition itself, so would this approach still work there? Or would the spec class still be loaded too early in this case? This may take a little more experimentation, but I'd love to solve both problems at once if we can. I can do some experimenting with view models on Monday, or if you're up for it you can -- I'd start by playing with TestViewModelSpec/TestViewModel to see if it's possible for constants declared in those files to refer to columns similar to your DEFAULT_ORDER test case.

Thanks again for the contribution!

[lnikkila]

I noticed that other projects under the Yahoo organisation have a CLA bot running, and I signed the CLA before submitting the pull request.

It might be possible that the bot isn’t enabled for this particular repo.

I agree, the change should also be reflected in view models. I haven’t actually used them thus far, but I’ll take a look at them later this weekend if I can and see whether they need some additional work.

[sbosley]

Ah yes, you're right about the bot. I was expecting it to be a webhook of some kind, but apparently it's implemented as a github user watching the repository. So we're all good there!

I did do a very quick test -- even with this change view models still have the same problem because the model spec class is loaded as part of schema initialization, so we'll need a slightly different approach to handle such cases. I have a few vague ideas that might involve some slightly more complex changes to the code generator, but I'd welcome whatever suggestions/proposals you come up with! I'll do some more in-depth experimenting on Monday too.

[sbosley]

Hey @lnikkila, not sure if you had any more time this weekend to think about this, but I threw together one possible solution this morning -- would love to get your feedback or see if you have any alternative ideas.

My thought was to extend the code generator to allow it to copy constants from an inner class of the model spec. Even if the model spec class is loaded early like it is for view models, an inner class containing only constants wouldn't be loaded until after the schema declaration given your change in this PR. It would look something like this:

@ViewModelSpec(className="MyView", viewName="my_view")
public class MyViewSpec {
    public static final StringProperty A_STRING = SomeModel.SOME_STRING_PROPERTY;

    @Constants
    public static class Const {
        public static final Order DEFAULT_ORDER = MyView.A_STRING.asc();
    }
}

// which would generate...
public class MyView extends ViewModel {
    // Schema initialization goes here, loads MyViewSpec class
    // ...

    // --- constants
    // deferred initialization of inner const class
    public static final Order DEFAULT_ORDER = MyViewSpec.Const.DEFAULT_ORDER;
}

Note that constants declared in the model spec class itself would still be copied to the generated class and your change to do them after the schema would still apply -- this would really just be a workaround for the view model case.

This seems to solve the problem -- any comments on the approach/API or any alternative ideas? @jdkoren would you also like to weigh in? If we like that approach I'll merge this PR and open a new one with these inner class enhancements. Happy to consider other options too though.

[jdkoren]

@sbosley That seems like a fine solution. Thanks for looking into it, and thanks @lnikkila for the PR

[sbosley]

Ok, merging this and then I'll open another with the additional changes for view models. Thanks again @lnikkila!

[lnikkila]

Looks good to me. Thanks for implementing this!