Custom fields should be nested in resource objects in API

[vancluever]

Currently, PHPIPAM embeds custom field objects into the top-level resource in any specific API object. Consider this example of an address, where the field Custom1 has been added:

{                                              
  "code": 200,                                 
  "success": true,                             
  "data": {                                    
    "id": "11",                                
    "subnetId": "3",                           
    "ip": "10.10.1.10",
    "Custom1": "foobar"
  }                                            
}                                              

This, unfortunately, makes writing API integrations in Go, a strongly typed language, kind of fragile. As an example, check out the addresses.Address type type that I have created for phpipam-sdk-go. Here, I take advantage of Go's json standard library functionality to marshal the JSON data provided by the API response directly into a struct, rather than relying on an ambiguous map/dict/hash. This allows us to, among other things, document the integration better and communicate to the user exactly what needs to be set and what to look for when writing and reading data.

Unfortunately, when custom fields are added into the mix, since we cannot know ahead of time what they are, we start to have problems.

First off, they cannot be added to the resource because they are ambiguious and can change from installation to installation - one person might have Custom1, another might have Custom2 - we can't reasonably add these to addresses.Address so that these fields can be accommodated. Embedding basically presents the same problem - while these fields could be added to a separate struct and then wrapped with the base resource in a different structure, so all of the fields in addresses.Address and Custom1 and Custom2 were addressable at the same level, this still runs into issues when we don't know fields ahead of time and they need to be configurable by the user.

I have gotten around this in phpipam-sdk-go by adding functions that pre-fetch the schema from the API through, for example, /api/test/addresses/custom_fields/, and then having functions that either cull non-custom fields before returning a map, or verifying that a map of fields to set only includes custom fields and not non-custom ones. This process is expensive (multiple requests per operation), but it's ultimately the least invasive to accomplish what I'm looking for.

Now, the last problem is the fact that the statically typed struct does not work with required fields - you run into a constraint the way required fields are set is by adding the NOT NULL attribute the column in the altered table:

Error from API (500): Error: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'CustomTestAddresses' cannot be null

This basically breaks my integration when using required custom fields, and now anyone using my SDK or the Terraform plugin will need to ensure they are not using required custom fields.

What I would like to propose, is that instead or staying embedded in the root level resource (although they could most certainly stay there), custom fields become an object provided by the customFields field in the API resources, like so:

{                                              
  "code": 200,                                 
  "success": true,                             
  "data": {                                    
    "id": "11",                                
    "subnetId": "3",                           
    "ip": "10.10.1.10",
    "customFields": {
          "Custom1": "foobar"
    }
  }                                            
}                                              

This would streamline a lot of things in my SDK and I would imagine other things as well - I would be able to move custom fields to the main resource structures, remove extra custom field fetching/setting functionality, and resources that require custom fields could be manageable again (the constraint would still be hit, but at least one could specify custom fields on creation).

PS: It looks like some controllers (as of 1.2 anyway) are adding only field data that has been specified while others are adding null values on field data that is not specified. This has the effect where NOT NULL values (required values) get inserted with their default value (when they have a default) in controllers where the column is not specified, versus the behaviour above when the column is specified.

Example subnets INSERT:

INSERT INTO `subnets` (`subnet`, `mask`, `sectionId`, `masterSubnetId`, `isFolder`) VALUES ('168428288', '24', '1', '2', '0')

Example addresses INSERT:

INSERT INTO `ipaddresses` (`ip_addr`, `subnetId`, `description`, `dns_name`, `mac`, `owner`, `state`, `switch`, `port`, `note`, `is_gateway`, `excludePing`, `PTRignore`, `firewallAddressObject`, `lastSeen`, `CustomTestAddresses`) VALUES ('168427786', '3', 'foobar', NULL, NULL, NULL, '2', NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)

[phpipam]

Hi, I checked and I can add additional parameter that would change this:

stdClass Object
(
    [id] => 39531
    [subnetId] => 7
    [ip] => 173.194.112.1
    [is_gateway] => 
   ...
    [location] => 0
    [href] => href123
    [test] => test124
    [aaa] => 123
    [csid] => csid123
    [сверхчеловек2_2] => ru
)

to this:

stdClass Object
(
    [id] => 39531
    [subnetId] => 7
    [ip] => 173.194.112.1
    [is_gateway] => 
   ...
    [location] => 0
    [custom_fields] => Array
        (
            [href] => href123
            [test] => test124
            [aaa] => 123
            [csid] => csid123
            [сверхчеловек2_2] => ru
        )

)

Would this be ok ? It would be configurable via UI for API parameters.

[phpipam]

Also, custom_fields will always be present, even if none are defined - if so it will be null.

{
   "time" : 0.006,
   "code" : 200,
   "data" : {
      "id" : "21",
      "editDate" : null,
      "number" : "113",
      "description" : null,
      "custom_fields" : null,
      "domainId" : "1",
      "name" : "API"
   },
   "success" : true
}

With values:

      {
         "id" : "11",
         "name" : "EQC SYSA",
         "number" : "10",
         "editDate" : "2017-02-23 19:09:54",
         "description" : "http://mihap.si",
         "domainId" : "1",
         "custom_fields" : {
            "test" : "http://mihap.si",
            "url" : "111"
         }
      },

With multiple results:

{
   "time" : 0.006,
   "success" : true,
   "data" : [
      {
         "id" : "3",
         "name" : "gygygygy",
         "number" : "1111",
         "editDate" : null,
         "description" : null,
         "domainId" : "1",
         "custom_fields" : {
            "url" : null,
            "test" : null
         }
      },
      {
         "name" : "test_VRF_VLAN",
         "id" : "4",
         "number" : "123",
         "editDate" : "2016-02-03 19:09:08",
         "description" : null,
         "domainId" : "5",
         "custom_fields" : {
            "test" : null,
            "url" : "http://mihap.si"
         }
      },
...

[phpipam]

Please test.

[vancluever]

Hey @phpipam, this is working so far, in the sense that after I fixed api/index.php, things functioned, but there is an error in the way the new flag is being handed down:

echo $Response->formulate_result ($result, $time, $app->nest_custom_fields, $controller->custom_fields);

Should be:

echo $Response->formulate_result ($result, $time, $app->app_nest_custom_fields, $controller->custom_fields);

After changing this the nested custom field attributes loaded.

One bug I'm really seeing here: It looks like sub-object calls (ie: subnets/ID/addresses/ or sections/ID/subnets/ are 1) not removing the sub-object's non-nested custom fields and 2) displaying the root object's custom fields in the nested object's place. Example:

    {
      "id": "5",
      "subnetId": "3",
      "ip": "10.10.1.245",
      "is_gateway": "0",
      "description": "Gateway",
      "hostname": null,
      "mac": null,
      "owner": null,
      "tag": "2",
      "deviceId": null,
      "location": null,
      "port": null,
      "note": null,
      "lastSeen": "1970-01-01 00:00:01",
      "excludePing": "0",
      "PTRignore": "0",
      "PTR": "0",
      "firewallAddressObject": null,
      "editDate": null,
      "CustomTestAddresses": null,
      "CustomTestAddresses2": null,
      "custom_fields": {
        "CustomTestSubnets": null,
        "CustomTestSubnets2": null
      }
    }

The nested data is inconsistent as well as even when set in the subnet, they don't appear in the address item. Looking into what is going on here and then will send over a commit when I find it.

[vancluever]

Narrowed it down to the fact that formulate_result is given the custom field schema as set by the controller for any specific call. It looks like if the custom field schema in the controller is overridden during the call, it fixes things. Example in controllers/Subnets.php:

// addresses in subnet
if($this->_params->id2=="addresses") {
        $result = $this->read_subnet_addresses ();
        $this->custom_fields = $this->Tools->fetch_custom_fields ('ipaddresses');
...

Now yields the correct result:

    {
      "id": "5",
      "subnetId": "3",
      "ip": "10.10.1.245",
      "is_gateway": null,
      "description": "Gateway",
      "hostname": "",
      "mac": "",
      "owner": "",
      "tag": "2",
      "deviceId": "0",
      "location": "0",
      "port": null,
      "note": "",
      "lastSeen": "1970-01-01 00:00:01",
      "excludePing": "0",
      "PTRignore": "0",
      "PTR": "0",
      "firewallAddressObject": null,
      "editDate": "2017-04-17 17:40:27",
      "custom_fields": {
        "CustomTestAddresses": "foo",
        "CustomTestAddresses2": null
      }
    }

[vancluever]

@phpipam, PR is in with some fixes - if you want to review and if it looks good ~I think this is closeable, with a TODO to fix the aliased controller's makeup as documented in #1100.~

Spoke too soon! Just looking over the code and I don't think I saw anything regarding custom field stuff for POST and PATCH - correct me if I'm wrong? Would like that functionality too so that the nesting works both ways.

[vancluever]

Updated my PR with what I think is a good way to add this support in - check it out and let me know!

[phpipam]

Hi, merged https://github.com/phpipam/phpipam/pull/1100. Will leave this issue open for reference.

[vancluever]

Hey @phpipam - just updated our testing to use the latest commit off of master again, and it looks good! Thanks for your help and support on this!

Going to close this now as everything seems there - if something comes up I'll open a new issue or PR.