farmOS / farmOS.js

A JavaScript library for working with farmOS data structures and interacting with farmOS servers.
MIT License
15 stars 13 forks source link

Fix errors and stabilize `subrequests` API #71

Closed jgaehring closed 1 year ago

jgaehring commented 1 year ago

In #68 I mentioned how I was reluctantly merging the subrequest work b/c I had gotten a few key structural changes mixed in with the subrequest stuff and didn't want to sort it out, but it would be best to resolve that stuff before the general release of 2.0.0.

I got some great advice via https://github.com/farmOS/farmOS.js/pull/68#issuecomment-1284492621 from @paul121 (it totally worked, btw, huzzah!) on the weirdness I was encountering with the /${entity}/${bundle}/relationships/${field} endpoint, and also resolved a few other bugs along the way, but still more issues remain. Debugging this stuff is kind of excruciating and damn slow work, so I'm punting on it for now. I'll try to pick this up and detail those issues in greater detail here soon.

duarteoctavio commented 1 year ago

Can I join the effort, perhaps? We might find a less "excruciating" way of working with it also. It seems to be fairly complex, I was impressed by how well it works already.

jgaehring commented 1 year ago

So just to recap some things I discussed with @OctavioElMagnifico yesterday...

Last week I was able to surface some issues related to subrequests that were likely responsible for some of issues we've encountered. Chief among them was this little problem with parsing a subschema of type "string", "number" or any other non-object schema type, which includes a combinator keyword such as anyOf, allOf or oneOf, such as the inventory_adjustment attribute in quantities:

{
  // rest of the schema...
  "attributes": {
    // more attributes...
    "inventory_adjustment": {
      "type": "string",
      "title": "Inventory adjustment",
      "oneOf": [
        {
          "const": "increment",
          "title": "Increment"
        },
        {
          "const": "decrement",
          "title": "Decrement"
        },
        {
          "const": "reset",
          "title": "Reset"
        }
      ],
      "description": "What type of inventory adjustment is this?"
    }
    // etc...
  }
}

This was a simple enough fix with https://github.com/farmOS/farmOS.js/pull/81/commits/a26f246ed67c0f82d60a1f6dd7cb6d1a1ebf5e93. It definitely resolved some issues I was seeing where quantities in subrequests were not being created on the server but I wasn't getting clear errors or logs on why that wasn't happening.

Apart from that, I've also added a few commits to PR #83 that I just opened. They may not have been responsible for any specific errors, but will make the tests a bit more reliable (2f8b300), make for more consistent parsing of subresponse content ids (ecdf0b0), and just clean things up a bit internally (1674698).

@OctavioElMagnifico and I have also discussed #76 in more detail and agree that an alternative to the singular $create command, such as $createMany, will be required for one-to-many relationships, such as the quantity relationship on logs, or indeed most relationships in all entities. This too should be fairly simple to implement.

The biggest remaining issue, however, is one that I don't know can be entirely resolved without changes to farmOS itself or the underlying Drupal subrequests module. This has to do with how the server parses JSONPath tokens in subrequest blueprints, a process which seems to be in need of optimization. That's because it's creating an unnecessary number of subrequests for every possible combination of tokens in each blueprint, so the algorithmic complexity grows geometrically when its dependent upon more than one preceding subrequest. I've made a gist with a truncated example of this. Within the limits of what can be expressed with JSONPath, and all the required drupal_internal__* properties that must be added as JSONPath tokens when posting a new relationship, I don't see a way around this client-side.

The good news, though, is that I don't think this will be such a problem if we can just limit the complexity of the queries themselves, which seems perfectly reasonable. To that purpose, together with the aforementioned need for a $createMany command, I'm going to start working with a modified test query:

const testQuery = {
  $create: {
    type: 'log--input',
    name: 'west field bed 12',
    category: {
      $find: {
        type: 'taxonomy_term--log_category',
        name: 'pest_disease_control',
      },
      $limit: 1,
      $createIfNotFound: true,
    },
    quantity: {
      $createMany: [{
        type: 'quantity--standard',
        label: 'hhh',
        measure: 'weight',
        units: {
          $find: {
            type: 'taxonomy_term--unit',
            name: 'US_gal_acre',
          },
          $sort: {
            weight: 'DESC',
          },
          $limit: 1,
          $createIfNotFound: true,
        },
      }],
    },
  },
};

This excludes the owner and location relationships that were present in the previous test query. In preliminary trials, removing at least one relationship seemed to resolve the memory exceptions most of the time, but part of the dilemma here is there's no determinate limit on the length of the query that we can depend on, so I believe removing at least two relationships will give us a sufficiently complex query for testing w/o blowing the stack. As a sidenote, the owner relationship itself seemed to produce errors more frequently than others, but I haven't been able to get any conclusive info from the server as to why that is, apart from the fact that the owner is an entity of user type, so there may be permissions or other validation checks involved with creating a new one.

So if I can get the above query to work consistently, I trust we can consider this and related issues resolved, at which point I'll merge #83 for release in beta.16, hopefully by the end of next week or early next.

jgaehring commented 1 year ago

I spoke again with @OctavioElMagnifico and came up with an alternative that may require a few more round-trip requests to the server, but which I think will simplify everything on both the client and the server while still keeping the number of round-trips to an acceptable minimum.

The basic idea is to scrap the whole idea of a recursive subrequest object, and limit the subrequest to a flat object with no nesting, which can be included as an option with each entity's .send() method. The recursive aspects of our original subrequest implementation are what I think ultimately led to the memory issues, so this approach seems potentially a lot more reliable, and would also provide some clear boundaries for where to "break" a series of requests into logical "chunks" that can each be sent in a single round-trip of subrequest blueprints. Then, in order to accommodate multiple quantities (#76), we can overload the .send() method parameters to allow an array of entities, in addition to just a single entity.

So here's what this would look like:

const quantF = farm.quantity.create({
  type: 'quantity--standard',
  label: 'fff',
  measure: 'weight',
});
const quantG = farm.quantity.create({
  type: 'quantity--standard',
  label: 'ggg',
  measure: 'weight',
});
const quantH = farm.quantity.create({
  type: 'quantity--standard',
  label: 'hhh',
  measure: 'weight',
});
const quantities = [quantF, quantG, quantH];

const quantOptions = {
  /**
   * Use a method called `subrequest` that takes an entity parameter, corresponding
   * to each of the entities in the array being sent, and returns a query object.
   */
  subrequest(quant) {
    const unitName = quant.label === 'hhh' ? 'US_gal_acre' : 'US_gal';
    return {
      units: {
        $find: {
          type: 'taxonomy_term--unit',
          name: unitName,
        },
        $sort: {
          weight: 'DESC',
        },
        $limit: 1,
        $createIfNotFound: true,
      },
    };
  },
  /**
   * Alternatively, the subrequest option can be a simple object, which will be
   * applied as the subrequest for every quantity in the array being sent.
   */
  // subrequest: {
  //   units: {
  //     $find: {
  //       type: 'taxonomy_term--unit',
  //       name: 'US_gal_acre',
  //     },
  //     $sort: {
  //       weight: 'DESC',
  //     },
  //     $limit: 1,
  //     $createIfNotFound: true,
  //   },
  // },
};

const log = farm.log.create({
  type: 'log--input',
  quantity: quantities.map(q => ({ id: q.id, type: q.type })),
});

const logOptions = {
  subrequest: {
    location: {
      $find: {
        type: 'asset--land',
        status: 'active',
        land_type: 'bed',
        name: 'west field bed 12',
        is_location: true,
      },
      $limit: 1,
      $createIfNotFound: true,
    },
    category: {
      $find: {
        type: 'taxonomy_term--log_category',
        name: 'pest_disease_control',
      },
      $limit: 1,
      $createIfNotFound: true,
    },
  },
};

farm.quantity.send(quantities, quantOptions)
  .then(() => farm.log.send(log, logOptions));

I like this because it fits more nicely within the existing farmOS.js API w/o the need for additional methods or helpers, while also yielding some control back to the consumer to make certain decisions about how to structure their asynchronous calls. I still need to evaluate a few aspects of this, but for the most part, it seems like a good way to proceed.