Adds support for last_sync attribute for Indexed Fixtures #2085

shubham1g5 · 2019-02-27T06:45:03Z

Adds a column for root level attributes in IndexedFixtureIndex user DB table
Adds AndroidIndexedFixtureInstanceTreeElement to support attributes lookup for root attibutes.

ctsims · 2019-03-01T21:53:36Z

Hi Shubham,

It seems a bit limiting for us to only be able to store a single attribute on the base element.

A lot of the dispatch downstream seems quite complex: https://github.com/dimagi/commcare-core/pull/842/files#diff-de4be0557361343fe019d6c9b25d8598R111

I'm wondering whether it would make more sense to instead serialize and store a single TreeElement which contains all of the core attributes. That element could be deserialized in whole, and then all of the attribute dispath methods in the element https://github.com/dimagi/commcare-core/pull/842/files#diff-de4be0557361343fe019d6c9b25d8598R95 could just be forwarded along, IE

    public String getAttributeName(int index) {
        return attributeHolderElement.getAttributeName(index)
    }

that would allow us to basically keep track of whatever other details need to be added in the future as well

shubham1g5 · 2019-03-20T11:07:30Z

@ctsims This is ready to be reviewed again.

app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java

app/src/org/commcare/models/database/IndexedFixturePathUtils.java

ctsims · 2019-03-22T09:39:54Z

app/src/org/commcare/models/database/IndexedFixturePathUtils.java

+                TreeElement attrsHolder = null;
+                byte[] attrsBlob = c.getBlob(c.getColumnIndexOrThrow(INDEXED_FIXTURE_PATHS_COL_ATTRIBUTES));
+                if (attrsBlob != null) {
+                    attrsHolder = SerializationUtil.deserialize(attrsBlob, TreeElement.class);


Additionally, I think we should have a concrete exception handling strategy for issues here, since deserialization exceptions are probably a very unexpected outcome.

It might be worth doing something like retrieving the byte[] here, and letting the TreeElement get deserialized at a later point (like in get() here: https://github.com/dimagi/commcare-core/pull/842/files#diff-de4be0557361343fe019d6c9b25d8598R38) when we are actively doing reads/deserializations from storage more broadly?

Additionally, I think we should have a concrete exception handling strategy for issues here, since deserialization exceptions are probably a very unexpected outcome.

Do you mean that we don't want the app to crash on such deserialization exceptions and instead show some sort of error message to the user ?

Mostly I mean that having the app crash here in a deserialization exception would be extremely confusing because it would be a symptom of an event that's out of the user's control and would be handled in a very strange part of the lifecycle.

Generally speaking I think it's very, very dangerous for "Utility" classes like this which are static and effectively stateless to throw errors that aren't strongly meaningful at the time. To the programmer, this method looks like it's just receiving a db hook and a string and doing some lookups. The caller wouldn't be expecting to need to be ready to handle mismatched serialized internal models based on the signature and expectations.

That would be their expectation later, however, when they are actually managing the resulting lookup table, so pushing the deserialization code there might be cleaner.

@ctsims Thanks. I get your point now. Though I am struggling to find a better place for the deserialization to go since it requires access to the Prototype factory and hence needs to be in the commcare-android. The get function you linked above is part of commcare-core. The only other place I can forsee is 1 level up i.e. getIndexedFixtureIdentifier but it again has the same semantics issue that doesn't convey to the caller to handle deserialization.

Alternatively, we can decide to not catch DeserializationException in SerializationUtil.deserialize which would add a throws DeserializationException into function signature and hence will be much more evident to a caller function.

Hm, I see what you are saying.

I can't think of a great workaround for now, and it is true that if we broke the TreeElement serialization, we'd need to be fixing the records from scratch anyway.

Maybe the big point of confusion here is just that it doesn't feel right for the attribute holder to be in this record to begin with. Can we make a second version of this method that doesn't pull the attrsHolder so that if we needed to just get the base and child paths, that there's a path for that? Would be good for this version to also throw a typed error specific to this problem so the end user (and us) can see very deliberately that it's an issue with the stored format for retrieving this.

I'm thinking about what pieces we'll need to have in place in a circumstance where we need to migrate fixtures due to a TreeElement serialization format change, and right now this method would just be a blocker.

@ctsims

That makes a lot of sense though just to clarify are you suggesting that we create an unused function/ code path right now because we might need it in future ?

shubham1g5 · 2019-04-03T09:58:53Z

@damagatchi retest this please

shubham1g5 · 2019-04-03T10:07:26Z

@damagatchi retest this please

shubham1g5 · 2019-04-03T10:12:10Z

@damagatchi retest this please

shubham1g5 · 2019-04-03T10:18:52Z

@damagatchi retest this please

ctsims · 2019-04-17T18:58:53Z

@shubham1g5 I think we are both aligned on potential ways forward here, and sorry for all of the back and forth. i'm fine with whatever approach you want to take based on our conversations

shubham1g5 · 2019-04-19T11:58:46Z

@damagatchi retest this please

shubham1g5 · 2019-04-19T12:06:43Z

@damagatchi retest this please

shubham1g5 · 2019-04-22T09:17:12Z

@ctsims This is ready to get reviewed again. Can you take another look.

ctsims · 2019-04-22T14:59:32Z

app/src/org/commcare/utils/AndroidInstanceInitializer.java

    }

+    @Override
+    protected AbstractTreeElement setupFixtureData(ExternalDataInstance instance) {


@shubham1g5 We should make sure that we've also implemented anything we need to implement here for Formplayer.

This may require, for example, require us to bump the version DB (which requires us to clear out Incomplete Forms).

Unrelated to this PR structure, but useful for us to know whether we need to update the Formplayer DB Version # with the next commcare-core pull in that library before merging this one (let me know if this comment doesn't make sense).

ctsims

Hey, Shubham,

This looks great. Two small changes specifically in the Lazy Deserialization method, and one higher level question: Have we piloted to make sure FormPlayer works, and whether we need to apply a Version bump before it does (IE: It works out of the box, but if someone has already restore/loaded an indexed fixture, does it work to deserialize that first sandbox)

ctsims · 2019-04-22T15:09:02Z

app/src/org/commcare/engine/cases/AndroidIndexedFixtureInstanceTreeElement.java

+        return getDeserializedAttributes().getAttributeValue(namespace, name);
+    }
+
+    private TreeElement getDeserializedAttributes() {


This method needs to be synchronized in the same way as this one:
https://github.com/dimagi/commcare-core/blob/master/src/main/java/org/commcare/cases/instance/StorageInstanceTreeElement.java#L103

to ensure that access to the attributes class is properly managed between threads.

We may want to consider a similar approach to that class where we have one "lazy load method" (there it's called loadElements()) that's called upfront before any methods, rather than specifically presuming that the lazy load is around the attributes.

Made the method synchronized here though I am not sure of any particular benefits of doing deserialization ahead of time, specially since it requires us to be mindful of not accessing the attributes before we call the load on them. For ex. even though loadElements() is designed to be called once at the start we still end up calling it at couple other places.

Hi Shubham,

I think the lazy loading independent of attributes is just a stylistic thing, so fine for it to not be included.

-Clayton

ctsims · 2019-04-22T15:11:53Z

app/src/org/commcare/engine/cases/AndroidIndexedFixtureInstanceTreeElement.java

+
+    private TreeElement getDeserializedAttributes() {
+        if (attributes == null) {
+            attributes = SerializationUtil.deserialize(attrHolder, TreeElement.class);


I think we may want to wrap serialization errors here, and specifically rethrow the error (still as a runtimeexception) with a message that says something like "Fixture Root Element Wrapper deserialization failed:" along with the real message, just so when this is seen and reported, we know exactly what is at error.

shubham1g5 · 2019-04-22T17:33:03Z

@ctsims I have not checked it on Formplayer yet though assume it will require some changes.

Do we want to add this root level attribute lookup support on formplayer as well ? (This will require a similar implementation asAndroidIndexedFixtureInstanceTreeElement on formplayer level)
Or Is the concern here that the current core changes should not break existing functionality on Formplayer ? (This will require the DB upgrade for IndexedFixtureIndex table)

ctsims · 2019-04-22T17:46:31Z

@shubham1g5

I believe that we now should consider ourselves committed to making sure that core engine functionality such as this is implemented on both sides of the platform, since people primarily build and test their apps on Formplayer.
Yes. My concern is specifically that we no longer have a clear process for determining when we can push commcare-core changes to FormPlayer, so if I, say, wanted to pull Fix for Short Cycle Detection Algorithm commcare-core#851 into formplayer's commcare-core branch, we aren't tracking data model changes that will require a UserDB version upgrade.

On the #2 point there - We don't need to migrate formplayer's db. I am fine continuing to kill the UserDB entirely by updating the version, but I don't think we should know when we are making changes that result in commcare-core being unpullable in FormPlayer in case we needed to manage an urgent issue.

shubham1g5 · 2019-04-22T18:39:04Z

@ctsims Nice points.

I think that also mean that we should push related changes for both Mobile and formplayer in the same mobile release cycle which essentially means parallel development for both. Though to support such process in long term, we will need to establish that everyone who works on mobile also is responsible for corresponding feature development on formplayer.
You are right that today we need to be careful to examine any DB changes in the core PR before merging it in formplayer. I am not sure if it's an overkill to give some label to these kind of PRs to make it more evident.

shubham1g5 · 2019-04-26T08:35:05Z

@ctsims bump to look at last set of changes here.

ctsims · 2019-04-26T14:29:02Z

good from my side!

shubham1g5 added 3 commits February 22, 2019 12:42

Adds last_sync col to Indexed fixture index table

e016043

last sync parsing

36ef3bb

Adds last sync in projection

520e678

shubham1g5 added enhancement cross requested product/internal-or-flagged labels Feb 27, 2019

shubham1g5 changed the title ~~Adds support for last_sync attribute for Indexed Fixtures~~ Adds support for last_sync attribute for Indexed Fixtures Feb 27, 2019

shubham1g5 mentioned this pull request Feb 27, 2019

Last sync support for Indexed Fixtures dimagi/commcare-core#842

Merged

shubham1g5 requested a review from ctsims February 27, 2019 06:45

Supports all kinds of attributes on indexed fixtures

faf96c6

ctsims requested changes Mar 22, 2019

View reviewed changes

shubham1g5 added 2 commits March 25, 2019 15:51

Moved the index fixture constants back to core, code cleaup

d33e45c

Cleanup

b3e7d95

shubham1g5 added 2 commits April 19, 2019 16:22

Lazily deserializes indexed fixture attributes

6051707

minor cleanup

4563659

shubham1g5 added 4 commits April 22, 2019 01:10

Adds test for index fixture root attribute lookup

fc52d3d

Creates Android instance initializer with right test sandbox

4829cc9

Better constructors

62e5a8e

Corrects xpath instance

66dac55

ctsims reviewed Apr 22, 2019

View reviewed changes

ctsims requested changes Apr 22, 2019

View reviewed changes

Makes loadAttributes synchronized, better error

6a18511

ctsims approved these changes Apr 26, 2019

View reviewed changes

shubham1g5 merged commit d631b5e into master Apr 29, 2019

shubham1g5 mentioned this pull request Jul 8, 2019

Adds Support for Root level attribute lookup for Indexed Fixtures dimagi/formplayer#681

Merged

Uh oh!

Adds support for last_sync attribute for Indexed Fixtures #2085

Adds support for last_sync attribute for Indexed Fixtures #2085

Uh oh!

Conversation

shubham1g5 commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctsims commented Mar 1, 2019

Uh oh!

shubham1g5 commented Mar 20, 2019

Uh oh!

Uh oh!

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!

shubham1g5 commented Apr 3, 2019

Uh oh!

shubham1g5 commented Apr 3, 2019

Uh oh!

shubham1g5 commented Apr 3, 2019

Uh oh!

shubham1g5 commented Apr 3, 2019

Uh oh!

ctsims commented Apr 17, 2019

Uh oh!

shubham1g5 commented Apr 19, 2019

Uh oh!

shubham1g5 commented Apr 19, 2019

Uh oh!

shubham1g5 commented Apr 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctsims 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!

shubham1g5 commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctsims commented Apr 22, 2019

Uh oh!

shubham1g5 commented Apr 22, 2019

Uh oh!

shubham1g5 commented Apr 26, 2019

Uh oh!

ctsims commented Apr 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shubham1g5 commented Feb 27, 2019 •

edited

Loading

shubham1g5 commented Apr 22, 2019 •

edited

Loading