fix(ui): Fix incorrect steps to remove ensembler fields

[deadlycoconuts]

Context

Certain fields in a router's ensembler config need to be removed manually before being stringified into a JSON object that will be sent to the Turing API server as the payload in various HTTP calls (e.g. update router, create new router version, etc.):

• ensembler (the entire ensembler config)
• ensembler.standard_config.fallback_response_route_id
• ensembler.docker_config
• ensembler.docker_config.resource_request.cpu_limit / ensembler.pyfunc_config.resource_request.cpu_limit

This was previously handled in the toJson method of the a TuringRouter object which gets called when JSON.stringify gets called on the TuringRouter object. However, in reality, this did not remove any of the ensembler configs listed above.

When JSON.stringify gets called on an object (TuringRouter) that has nested objects which also have their own toJson methods implemented (Ensembler), JSON.stringify actually calls toJSON recursively from the original object all the way down to their nested objects. However, because we actually perform a deep copy of the TuringRouter object in toJSON, we actually also copy a bounded toJSON belonging to its ensembler. As such, even when we try to delete fields in the deep copied router's ensembler, nothing happens because the bounded toJSON gets called, and returns all fields as part of the original ensembler.

For example, given an instance of the class Parent:

class Parent {
  constructor() {
    this.name = "David";
    this.child = new Child();
  }

  toJSON() {
    // this is done to deep clone the Parent object
    const tmp = {};
    tmp.name = this.name;
    tmp.child = { name: this.child.name, toJSON: this.child.toJSON };

    // this does not delete anything because even though tmp.child.toJSON will be called to return the JSON string
    // for the child attribute, tmp.child.toJSON is actually bounded to this.child which does not have its name
    // deleted, thus the name will still show up in the final output
    delete tmp.child.name;
    return tmp;
  }
}

class Child {
  constructor() {
    this.name = "Goliath";
    // this binding ensures that anyone that copies the toJSON method will always call toJSON on this specific instance
    this.toJSON = this.toJSON.bind(this);
  }

  toJSON() {
    // does nothing; just returns all fields faithfully
    return this;
  }
}

someone = new Parent();
console.log(JSON.stringify(someone)); 

// OUTPUT: {"name":"David","child":{"name":"Goliath"}}

As such, this is some conflicting behaviour as a result of us performing both the deep-cloning AND the binding of toJSON, which (unexpectedly or not) caused the bounded toJSON function to be cloned and used. Of course we can either: 1) not clone the bounded toJSON function, but this is hard since we're using a deep cloning library, and if we don't clone toJSON, we still need to replicate the logic contained within it, or 2) don't bind toJSON but I don't know what the original intent of the binding is for, and removing it might break other stuff unexpectedly.

As such, I'll simply move all the ensembler-related deletion logic into the toJSON method of the ensembler to ensure that all the necessary fields get removed properly, without having to deal with either the cloning library or the binding.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.27%. Comparing base (cbebbd6) to head (3565471).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #382      +/-   ##
==========================================
- Coverage   68.07%   62.27%   -5.80%     
==========================================
  Files         149      124      -25     
  Lines       11809     9774    -2035     
==========================================
- Hits         8039     6087    -1952     
+ Misses       3032     2949      -83     
  Partials      738      738              

Flag	Coverage Δ
api-test	62.27% <ø> (ø)
sdk-test-3.10	?
sdk-test-3.8	?
sdk-test-3.9	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[deadlycoconuts]

This entire check was ineffective and was doing nothing since it was checking a field that doesn't exist - obj.config.ensembler.resource_request does not exist. The intended path should've been obj.config.ensembler.docker_config.resource_request or obj.config.ensembler.pyfunc_config.resource_request.

[leonlnj]

The delete is done is ensembler.is instead?

[deadlycoconuts]

Yep that's right, because toJSON in Ensembler.js gets called after the entire return value of toJSON in TuringRouter.js gets returned.

[leonlnj]

thanks