FLUID-5585: Removals from model are never relayed using model relay system

Description

When using "fluid.transforms.free" to relay the model change to the target, the removal from the source model doesn't get relayed.

For example, if the initial model starts with:
{
a: true,
b: true
}

Then, modify the model to
{
a: true
}

The modification doesn't get relayed to the target.

Comments

• Cindy Li commented 2015-01-13T11:29:30.444-0500

  Sent a pull request with a unit test to demonstrate this issue: https://github.com/fluid-project/infusion/pull/576

• Antranig Basman commented 2015-01-13T17:35:08.423-0500

  In fact the relay system is deficient in relaying removal of values of all kinds - the "DELETE" ChangeRequest type has been a 2nd-class citizen for a while and is not properly recognised by any of the relay system. This posed quite a profound-seeming problem at first - discussion in IRC at - https://botbot.me/freenode/fluid-work/2015-01-13/?msg=29435729&page=1

  Relevant messages pasted here:

  Bosmon: I think we are going to have abandon a previously held dear religion, and start permitting change requests with a value of "undefined" ......
  We'll just have to take care to "armour" them back into DELETE requests whenever we try to transmit them to anyone or store them
  cindyli: ok. but why would that require to permit change requests with a value of "undefined"
  Bosmon: cindyli - well, we need a way to propagate the information about deletions through essentially arbitrary code
  For example, the code fluid.identity
  Now fluid.identity is hardly going to response to a change request of type "DELETE"
  It has no idea what such things are
  The only thing it can respond to is VALUES.... which implies we must have a VALUE WHICH REPRESENTS A DELETION
  7:24 PM cindyli:ok
  7:24 PM Bosmon: Now in JavaScript, our reasonable choices for such a value are extremely limited
  7:24 PM In theory we could send in some special "exotic" value that we could check with a prototype or constructor check
  7:24 PM But this seems rather antireligious in some other ways
  7:25 PM "undefined" has the virtue of meaning almost exactly what we mean - in that after servicing a DELETE, undefined really is the value which we would observe after a read
  7:26 PM It would just require special processing in the changeApplier to honour it with a language-level "delete" rather than sticking it in as a value
  7:26 PM It would mean that the changeApplier then loses the ability to operate on "broken JSON" trees which hold "undefined" as property values, but it never really had this anyway
  7:27 PM Actually as the recent GPII settings handler problems demonstrated, 100% of our architecture, as well as our base libraries such as jQuery, lack the ability to process trees like that anyway
  7:36 PM Well, actually we are stumped
  7:36 PM We have already chosen to use "undefined output value" our convention within the model transformation system to mean "do not propagate any change to output"
  7:44 PM ok, LOO TIME has brought results
  7:45 PM I started to think about falling back onto the "exotic value" system
  7:45 PM but this is also obviously unworkable
  7:45 PM Imagine something like fluid.transforms.linearScale encountering a value OF ANY KIND representing a deletion
  7:46 PM It's obviously never going to have any idea what to do with it
  7:46 PM Unless we special-case the code in each of our transforms, they're obviously going to mishandle it
  7:47 PM So, this obviously has to be handled (to start with) outside the reach of the model transforms system entirely
  7:48 PM Initially it should just be a primitive of the relay system - that any relay rule that receives a DELETE at source/target should just transfer this by default to a DELETE at the other ened
  7:49 PM In the future, I guess, we will have to add in some special casing for oddities like transforms which transform undefined ones into defined ones
  7:49 PM For example, the valueMapper does this in some configurations
  7:51 PM cindyli: ok, i see
  7:52 PM Bosmon: Well ok, thinking further, I think I realise we need to do even more
  7:53 PM EVERY relay rule, accepting ANY value of any kind, should immediately begin by deleting the value at the other end - if it thinks it is going to try to relay it
  7:53 PM Only if it succeeds in reestablishing it should the new value be accepted
  7:53 PM Actually I think this succeeds in solving all of our problems immediately
  7:53 PM Including several that we haven't run into yet
  7:54 PM However, trying to implement this new aggressive behaviour may well expose several other bugs lying in the system
  7:55 PM We have been relying, I think, as a kind of "safety net" that the default behaviour of the system is to do nothing, if it encounters some problem with a relay rule
  7:55 PM But the default behaviour of the system will now be to utterly destroy the value at the other end of a relay rule : P
  8:03 PM cindyli: ya, sounds a bit risky. would be safer to save the destroyed value for a future restore when a relay fails
  8:04 PM Bosmon: Well
  8:04 PM We will just have to try it
  8:04 PM Either the relay result is correct or it isn't : P
  8:05 PM We can't have an architecture which depends on accidental "do-nothing" behaviour...
  8:13 PM oh dear
  8:13 PM It appears that an ARCHITECTURAL PROBLEM prevents this simple solution from being very simple
  8:14 PM I may have to just shelve this as yet another problem to be solved when we rewrite the relay system
  8:14 PM Unfortunately the system "has no idea" at the level of change events when an instance of relay is beginning and ending
  8:14 PM What a stupid system
  8:15 PM All of the relays all funnel through fluid.registerDirectChangeRelay
  8:15 PM Which all it does is connect an incoming stream of changes on a target
  8:15 PM This makes it very hard to figure out when to apply the "punctuation" of sending in the DELETE ....
  8:17 PM cindyli: the good thing is, this jira is not blocking anything. we have time
  8:19 PM also, another change applier jira is on its way, you may wanna see that first before coming up with the solution
  8:19 PM Bosmon: ok
  8:19 PM Well, it may be possible to find a quicker path by special-casing the two kinds of transforms
  8:19 PM I've found the place to punctuate transforms which use a full "modelRelay" record
  8:20 PM Bosmon: And if this doesn't cause everything to explode, I can try to extend this to relays which just use implicit relay
  8:32 PM Bosmon: Hey cindyli
  8:32 PM I can now pass your test case : P
  8:32 PM cindyli: what did you do
  8:32 PM Bosmon: And amazingly it seems that nothing else blew up
  8:33 PM I will now extend your test case and my fix to deal with the "implicit relay" case
  8,33 PM cindyli: awesome. thanks, Bosmon
  8:34 PM Bosmon: https://gist.github.com/amb26/5fcc3816dbd103bdb45d
  8:34 PM Here's my current diff ....

• Tony Atkins [RtF] commented 2016-10-17T07:54:15.283-0400

  Hi, @@Antranig Basman and @@Cindy Li. I have recently been bitten by this problem, as the gpii-binder uses the "DELETE" type to clear out model variables when form inputs are cleared.

  Check out this example for a disturbing variation on the problem:

  /* eslint-env node */
  "use strict";
  var fluid = require("infusion");
  
  fluid.defaults("fluid.tests.deleteRelay", {
      gradeNames: ["fluid.modelComponent"],
      model: {
          deep: {
              parentValue: true,
              parentDelete: true,
              childValue: true,
              childDelete: true
          }
      },
      components: {
          child: {
              type: "fluid.modelComponent",
              options: {
                  model: "{deleteRelay}.model"
              }
          }
      }
  });
  
  var component = fluid.tests.deleteRelay();
  
  console.log("parent model before change:", JSON.stringify(component.model, null, 2));
  console.log("child model before change:", JSON.stringify(component.child.model, null, 2));
  
  component.applier.change("deep.parentValue", "new parent value");
  component.applier.change("deep.parentDelete", "new parent value", "DELETE");
  
  console.log("parent model after parent delete:", JSON.stringify(component.model, null, 2));
  console.log("child model after parent delete:", JSON.stringify(component.child.model, null, 2));
  
  
  component.child.applier.change("deep.childValue", "new child value");
  component.child.applier.change("deep.childDelete", "new child value", "DELETE");
  
  console.log("parent model after child delete:", JSON.stringify(component.model, null, 2));
  console.log("child model after child delete:", JSON.stringify(component.child.model, null, 2));
  

  This results in output like the following:

  parent model before change: {
    "deep": {
      "parentValue": true,
      "parentDelete": true,
      "childValue": true,
      "childDelete": true
    }
  }
  child model before change: {
    "deep": {
      "parentValue": true,
      "parentDelete": true,
      "childValue": true,
      "childDelete": true
    }
  }
  parent model after parent delete: {
    "deep": {
      "parentValue": "new parent value",
      "childValue": true,
      "childDelete": true
    }
  }
  child model after parent delete: {
    "deep": {
      "parentValue": "new parent value",
      "parentDelete": true,
      "childValue": true,
      "childDelete": true
    }
  }
  parent model after child delete: {
    "deep": {
      "parentValue": "new parent value",
      "childValue": "new child value",
      "childDelete": true,
      "parentDelete": true
    }
  }
  child model after child delete: {
    "deep": {
      "parentValue": "new parent value",
      "parentDelete": true,
      "childValue": "new child value"
    }
  }
  

  Please note that on the next subsequent change to the child model, the variable that was deleted in the parent is recreated in the parent with the original value. This is the kind of thing that is likely to come up often in even moderately complex groups of components that use the binder.

  I could use a pairing session to discuss this, if there is no one working on it, I will likely need to address this before my work with the UL can continue.

• Tony Atkins [RtF] commented 2016-10-25T07:34:18.098-0400

  @@Justin Obara, I'm not sure why you changed the affects version, perhaps to get it off of some rollup list? IMO it would be better to set the "Affects Version" to a recent release, and to set the "Fix Version" once a release with a verified fix is available.

• Justin Obara commented 2016-10-25T09:13:12.004-0400

  @@Tony Atkins [RtF] the current intention is that this will be fixed for the Infusion 2.0 release, although that may change based on some discussions today. If the plan is changed, it will be reverted back to affecting 2.0 and the fix for removed or set to some future version.

• Tony Atkins [RtF] commented 2016-10-26T04:13:20.077-0400

  In later discussions it was agreed that it wouldn't be fixed in 2.0: https://github.com/fluid-project/infusion/pull/577#issuecomment-255371232

  I see that @@Justin Obara put the right "Affected Version" back in already, so we're good to go.

• Cindy Li commented 2017-01-06T10:28:40.233-0500

  The pull request https://github.com/fluid-project/infusion/pull/577 has been merged into the infusion repo master branch at 07cc0d5ba6fe85a4b910751c9db8cec6c501e3bc