Skip to content
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

Making required properties on Device Schema nullable. #148

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

zemberdotnet
Copy link
Member

Description

This PR makes the required Device properties, category, make, model, and serial, null-able properties.

Rationale

In graph-rumble, there are rumble_asset entities. These assets are all networked devices such as servers, desktops, phones, etc. These entities would be best classified as Devices. However, the response from Rumble API does not contain enough information to fill the required properties. Information about the category, make, and model are sometimes available, but serial could never be populated with the data from the response.

If a rumble_asset cannot use the Device schema, then the entity would be misclassified. By making required properties optionally null-able, the data model can encourage using common properties when available while retaining the flexibility to classify entities correctly when not all information is available.

There are other ways of achieving the same goal, so any ideas or feedback is appreciated!

Related Issues

Previous discussion on allowing all properties to be null during graph validation: #13
Possibility of using data-model to generate types for typescript or any other typed language: #12

References

graph-rumble repo
rumble_asset pull request
example Rumble asset response

@zemberdotnet zemberdotnet requested a review from a team March 4, 2022 16:20
@mdaum
Copy link
Contributor

mdaum commented Mar 4, 2022

Did we consider removing the contents of the required array? What would the implications of that be as opposed to making each required field nullable?

@zemberdotnet
Copy link
Member Author

Removing the required properties is another excellent option. Making the properties not required will prevent polluting the data model and integrations with `null's. It is also probably more straightforward, but I think there might be an opportunity here.

The more the standard lexicon of properties the data-model provides is used, the more value we can receive from it. As customers use the platform, I would want them not to be surprised by naming. There should be as little mental overhead to finding the name of a property, and ideally, they should even be guessable with some exposure to the platform.

When properties are not required, I expect they become more of a guide. I was working to measure how often public integrations use unrequired properties, but there is still more work to be done there.

Using an optional null makes the data-model opt-out rather than opt-in. My thought is that having better standards in place with opt-out will support more factory-style integration development and uniformity across the product.

This comes with the disclaimer that I don't have enough experience to say how often the data-model optional properties are being used in integration projects. This change also comes from an integrations development perspective. I'm unsure how others internally and in the open-source community use this project.

@austinkelleher Do you have any insight into how often optional properties are used in graph-* projects?

@zemberdotnet zemberdotnet merged commit 4b60820 into main Mar 4, 2022
@zemberdotnet zemberdotnet deleted the device-schema-nullable branch March 4, 2022 21:36
@austinkelleher austinkelleher mentioned this pull request Mar 6, 2022
@aiwilliams
Copy link
Contributor

Excellent write-up @zemberdotnet. I think it good to see, in code, that the integrations are opting out, or to put it another way, folks should be required to make a decision about excluding properties that are considered useful for certain activities and UIs in JupiterOne. I would look for comments around property: null that explain something like, "This cannot be determined in the data accessible through the APIs of the source system. See http://provider.com/docs/api-for-resource."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants