-
Notifications
You must be signed in to change notification settings - Fork 722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom fields should be nested in resource objects in API #1073
Comments
Hi, I checked and I can add additional parameter that would change this:
to this:
Would this be ok ? It would be configurable via UI for API parameters. |
Also, custom_fields will always be present, even if none are defined - if so it will be null.
With values:
With multiple results:
|
Please test. |
Hey @phpipam, this is working so far, in the sense that after I fixed
Should be:
After changing this the nested custom field attributes loaded. One bug I'm really seeing here: It looks like sub-object calls (ie:
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. |
Narrowed it down to the fact that
Now yields the correct result:
|
@phpipam, PR is in with some fixes - if you want to review and if it looks good 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. |
This release adds support for tracking a `dev` version of PHPIPAM. This is a commit off of master, that will hopefully end up being version 1.3 at some point in time. This version also currently tracks PBP's fork at https://github.com/paybyphone/phpipam while the nested custom field issues outlined in phpipam/phpipam#1073 and phpipam/phpipam#1100 are fixed. This also adds the gd PHP package to the build process to ensure that VRFs can be used.
Updated my PR with what I think is a good way to add this support in - check it out and let me know! |
Hi, merged #1100. Will leave this issue open for reference. |
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. |
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: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 haveCustom2
- we can't reasonably add these toaddresses.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 inaddresses.Address
andCustom1
andCustom2
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:
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:
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:
Example addresses INSERT:
The text was updated successfully, but these errors were encountered: