Bug 900289 - Add foreign key pragma to FHR database and associated tests

[mcomella]

No description provided.

[mcomella]

Additional concern: What assumptions are we making about null values? HealthReportDatabaseStorage.ensureMeasurementInitialized never checks for a null string which I imagine would put null into the database. This test (and subsequent fix) can be added to this series of commits.

[mcomella]

Additional concern 2: Do we want to verify that old databases don't violate constraints? We can dbVersion++ and remove any nulls from violations there.

[mcomella]

Additional concern 3: Performance implications of foreign keys. I imagine it's negligible, but it might be worth investigating.

[rnewman]

You could skip the PRAGMA for read-only DB connections. That would help with any perf issue. Write load is low-frequency enough that it's not a concern to me.

[rnewman]

And yeah, we should probably discard any inconsistent data in a migration.

[rnewman]

Heh, I guess you already did what I suggested :P

[rnewman]

This looks good to me. Take a look at migration/cleanup next, 'cos we'll want to land those together.

[mcomella]

Also, before I forget, please comment on #342 (comment). I feel like assertions would be useful here but I don't believe they're enabled by our build system. :(

[rnewman]

I'm not sure what you mean by assertions in this context; what can we check at build time?

[mcomella]

Like

assert measurementNames != null;

But I do not believe assertions are enabled in our build system. Alternatively,

if (measurementNames == null) {
    throw new IllegalStateException("measurementNames should not be null.");
}

Or better yet,

public void ensureMeasurementsInitialized(String[] measurementNames, ...) {
    ensureNonNull(measurementNames);
    // ...
}

private void ensureNonNull(Object obj) {
    if (obj == null) {
        throw new IllegalStateException("obj was null!");
    }
}

It'd probably be good to check for these things rather than assuming they'll never be null, though at the cost of some code complexity so I wanted to know if such checks are worthwhile.

[rnewman]

Yeah, the typical approach is to use exceptions during validation. E.g., https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/org/mozilla/gecko/sync/Utils.java#L209

If doing something will break things, the only three options are:

• Throw (validate)
• Ignore/log (leads to problems later, and relies on a web of contracts: what happens when I later try to retrieve my measurement by name?)
• Specify preconditions — e.g., in Javadoc,

@param measurementNames an array of non-null <code>String</code> instances.
@throw IllegalArgumentException if a <code>null</code> measurement name is provided.

In richer languages we wouldn't have this problem.

[mcomella]

In response to argument validation, without being overzealous, nothing I touched here seems to be a prime candidate for such validation but I'll keep an eye open in the future for places where it's applicable. Thanks for the edification.

[mcomella]

@rnewman: Ready for review if you didn't get the BZ notification.

[mcomella]

@rnewman: Ready for followup review.

[rnewman]

LGTM.