farmOS / field-kit

A modular, offline-first companion app to farmOS.
https://farmOS.org
GNU General Public License v3.0
59 stars 38 forks source link

Use `api/field_module/field_module` in `updateFieldModules` #480

Closed jgaehring closed 2 years ago

jgaehring commented 2 years ago

From https://github.com/farmOS/farmOS-client/issues/468#issuecomment-1004156578:

Interesting though, the call to https://test.farmos.dev/api/client_module/client_module 404'ed. Looking at the links at /api, I see that that endpoint was changed to https://test.farmos.dev/api/field_module/field_module, I assume when the farm_client module was changed to farm_fieldkit. Should be easy enough to fix. Also, I should make this configuration syncing process more resilient so a failed request like that doesn't cause the whole process to fail.

jgaehring commented 2 years ago

I'm realizing that the JavaScript file seems to have moved, too. It used to be:

https://test.farmos.dev/farm/client/js/test/index.js

Now it's:

https://test.farmos.dev/fieldkit/js/test/index.js

I think we talked about adding a uri attribute to the JSON object for each module that comes back from the api/field_module/field_module, but I guess it never got implemented. I would really like to have this added, so I can always trust if there are changes made in the server implementation those changes will be communicated via JSON:API.

If I had my druthers, this is what each module's resource object would look like in JSON:API:

{
  "type": "field_module--field_module",
  "id": "42ff2d3b-3c98-4d0b-a3c5-7e54800a1137",
  "links": {
    "self": {
      "href": "https://test.farmos.dev/api/field_module/field_module/42ff2d3b-3c98-4d0b-a3c5-7e54800a1137"
    }
  },
  "attributes": {
    "langcode": "en",
    "status": true,
    "dependencies": {
      "enforced": {
        "module": [
          "farm_fieldkit_test"
        ]
      }
    },
    "drupal_internal__id": "test",
    "label": "Test module",
    "description": "Just a field module for testing.",
    "uri": "fieldkit/js/test/index.js",
    "name": "test"
  }
}

Currently, there is also a library property, which for the test module has a value of "farm_fieldkit_test/test_field_module", but that's not really meaningful for any of Field Kit's use cases. I suppose I could parse the name out of that, and then from that try to rederive the uri, but that seems far too brittle, since it depends conventions that are only communicated out-of-band at this point. If we can guarantee that there will always be a uri for each module object at the api/field_module/field_module endpoint, then those are really the only 2 guarantees we need from the contract between the Field Kit API and the farmOS API. Everything else server-side, even the name, can change w/o breaking FK as long as we can always promise those two things.

paul121 commented 2 years ago

I think we talked about adding a uri attribute to the JSON object for each module that comes back from the api/field_module/field_module, but I guess it never got implemented.

Hey @jgaehring thanks for identifying this again. The TLDR is that we can't add computed fields to config entities, so adding "dynamic" properties (such as uri) into the field_module jsonapi resource is challenging.

This made me wonder about using the meta section of the resource, which led me to an open issue to support adding custom meta data: https://www.drupal.org/project/drupal/issues/3100732 Lots of nitty gritty there, but one specific comment here was of interest: https://www.drupal.org/project/drupal/issues/3100732#comment-13439744

JSON:API Hypermedia works by running an event dispatcher during normalization.

This reminded me of the JSON:API Hypermedia module which provides the ability to add custom links to the resource object - which is what we're trying to do here for field modules!! I think this could be a great (and easy!) solution!

Implementation is really quite simple - I added the following FieldModuleLinkProvider plugin:

<?php

namespace Drupal\farm_fieldkit\Plugin\jsonapi_hypermedia\LinkProvider;

use Drupal\Core\Access\AccessResult;
use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Url;
use Drupal\jsonapi_hypermedia\AccessRestrictedLink;
use Drupal\jsonapi_hypermedia\Plugin\LinkProviderBase;

/**
 * @JsonapiHypermediaLinkProvider(
 *   id = "farm_fieldkit.field_module.index",
 *   link_relation_type = "index",
 *   link_context = {
 *     "resource_object" = "field_module--field_module",
 *   }
 * )
 */
class FieldModuleLinkProvider extends LinkProviderBase {

  /**
   * {@inheritdoc}
   */
  public function getLink($context) {
    $field_module_id = $context->getField('drupal_internal__id');
    $url = Url::fromRoute('farm_fieldkit.field_module_js', ['field_module' => $field_module_id]);
    $cacheability = new CacheableMetadata();
    $access_result = AccessResult::allowed();
    return AccessRestrictedLink::createLink($access_result, $cacheability, $url, $this->getLinkRelationType());
  }

}

Which provides an index link for each field_module JSON:API resource:

"data": [
    {
      "type": "field_module--field_module",
      "id": "9cce513d-9b7d-44ff-bcaf-d7c8c0627dad",
      "links": {
        "describedby": {
          "href": "http://localhost/api/field_module/field_module/resource/schema"
        },
        "index": {
          "href": "http://localhost/fieldkit/js/test/index.js"
        },
        "self": {
          "href": "http://localhost/api/field_module/field_module/9cce513d-9b7d-44ff-bcaf-d7c8c0627dad"
        }
      },
      "attributes": {
        "langcode": "en",
        "status": true,
        "dependencies": {
          "enforced": {
            "module": [
              "farm_fieldkit_test"
            ]
          }
        },
        "drupal_internal__id": "test",
        "label": "Test module",
        "description": "Just a field module for testing.",
        "library": "farm_fieldkit_test/test_field_module"
      }
    }
  ],

You'll notice the describedby link is added here, too. This is actually provided by the jsonapi_schema module which already supports this jsonapi_hypermedia module!

SO my question for you @jgaehring describedby, index and self are all standardized link relations. I chose index because we are pointing to the field module "index.js", it just seemed natural. But is there a different link_relation_type you might prefer?

We also have the ability to provide our own link relation type in a farm_fieldkit.link_relation_types.yml file (an example: https://git.drupalcode.org/project/jsonapi_hypermedia/-/blob/8.x-1.x/jsonapi_hypermedia.link_relation_types.yml). We could do this, and something like field_module might make sense, but after seeing the standardized list I'm curious if we can't just use one of those instead!

mstenta commented 2 years ago

I see @jgaehring copied your comment to a new issue @paul121 : https://github.com/farmOS/farmOS/issues/468

Feel free to delete/truncate your comment above if we want to continue the discussion over there. :-)

jgaehring commented 2 years ago

I'm still in the middle of other changes and can't update my local server to test this out, but I've at least encapsulated these server details in a fetchFieldModules function, so it should be a snap to just update those strings when I'm ready to update:

https://github.com/farmOS/farmOS-client/blob/739a4941200135aee529ab014dcab22189e8a337/src/core/fieldModules.js#L82-L100

This isn't critical, but just out of curiosity, when (if ever) does a config entity's id change? I assume if it's fully uninstalled and then reinstalled, it would be granted a new id, but does it change when it's updated?

jgaehring commented 2 years ago

I'm still in the middle of other changes and can't update my local server to test this out

Actually, I realized I could just add a check to see if I was in development mode:

https://github.com/farmOS/farmOS-client/blob/55cc7aaa42baf98441b2eb1eecaaf0f79fd3915e/src/core/fieldModules.js#L82-L86

Works locally, and hopefully will on develop.farmos.app, too. I'll just need to remember to remove this at some point. Presumably whenever I upgrade my local server, it will break my local FK, so that will prompt me.

mstenta commented 2 years ago

Do you mean the drupal_internal__id (I generally refer to it as the "machine name" of the config entity, but that's sort of just a habit/Drupalism maybe... the drupal_internal__id name is only used in the JSON:API endpoints afaik)?

The machine name of a field module should never change... unless the module/developer that provides it decides to change it! :-P But that should be considered bad practice. It may happen, though... eg: if the eggs module decided to change to egg or something like that (just a hypothetical). We can avoid that change in any core field modules we provide, but can't really enforce it in contrib (we can document best practices and warn against it in docs though).

mstenta commented 2 years ago

Oh sorry just reread your question... maybe you mean UUID? The UUID will be automatically generated when the field module is first installed. It would only change if the module were uninstalled and reinstalled.

jgaehring commented 2 years ago

Do you mean the drupal_internal__id

Oh no, the actual id, aka UUID. I'm not using it for anything, but I am storing it just in case it comes in handy to differentiate modules.

jgaehring commented 2 years ago

Works locally, and hopefully will on develop.farmos.app, too.

Works there too!