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

Add optional schema property to graph object metadata #611

Merged
merged 3 commits into from
Feb 14, 2022

Conversation

ndowmon
Copy link
Contributor

@ndowmon ndowmon commented Feb 10, 2022

Added

  • Added optional schema property to StepGraphObjectMetadata. This allows
    developers to provide the property schema to expect on entities,
    relationships, and mapped relationships. This serves two uses:
    1. Schemas can be used at runtime or test-time to verify that an entity has
      the correct properties
    2. The j1-integration document command could automatically produce consumer
      documentation about the properties that an entity / relationship is
      expected to have

@ndowmon ndowmon requested review from aiwilliams, ceelias and a team February 10, 2022 15:13
Copy link
Contributor

@austinkelleher austinkelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Would you mind adding this to the development documentation please? I'd also love to see a task created to update the integration-template!

relationships, and mapped relationships. This serves two uses:
1. Schemas can be used at runtime or test-time to verify that an entity has
the correct properties
2. The `j1-integration document` command could automatically produce consumer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love to see a ticket in the SDK describing this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Would you mind adding this to the development documentation please? I'd also love to see a task created to update the integration-template!

I'd like to make some minor updates to toMatchDirectRelationshipSchema, toMatchGraphObjectSchema, and add the toMatchStepMetadata matcher we discussed previously. Then go back and update docs/testing and integration-template

Comment on lines +88 to +91
$schema?: string;
$id?: string;
description?: string;
additionalProperties?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to make some of these required? It could prevent us from bugs like what we saw in graph-aws

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. properties, required, additionalProperties

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export interface GraphObjectSchema extends IntegrationEntitySchema {
  $schema?: string;
  $id?: string;
  description?: string;
- additionalProperties?: boolean;
+ additionalProperties: boolean;
+ properties: Required<IntegrationEntitySchema>['properties'];
+ required: Required<IntegrationEntitySchema>['required'];
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the form of https://github.com/JupiterOne/data-model/blob/178d8838d6004a45ff97b229fd8a0dafd3b50e6f/src/index.ts#L6:

export type IntegrationEntitySchema = {
  $ref?: string;
  allOf?: IntegrationEntitySchema[];
  properties?: {
    [propertyName: string]: any;
  };
  required?: string[];
};

Nothing is required there, which makes sense, allowing for various combinations of those properties.

For this GraphObjectSchema, I think we could make description required. I'm confused about what $schema does here in the context of the metadata for an entity. Will $id ever be provided, perhaps we could remove that? We could probably use some documentation on these properties.

What do you think about naming this EntitySchemaExtension?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I think it's bad that we're not using the published NPM types for JSON Schema.

yarn add --dev @types/json-schema

import { JSONSchema7 } from 'json-schema'

Perhaps this type should actually be something like the following?

type GraphObjectSchema = Required<Pick<JSONSchema7, 'additionalProperties' | 'required' | 'properties'>>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I don't see why we would want to document a description if we're just using this to describe step entity metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I don't love EntitySchemaExtension because I would really like to see this used on relationships as well, in cases where relationships will have properties on them (example, permissions relationships or firewall rule relationships)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, nice find on the @types/json-schema! I wonder if I totally overlooked that or if it's relatively new. I think it would be wise to move to use those types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packages/integration-sdk-core/src/types/step.ts Outdated Show resolved Hide resolved
Comment on lines +88 to +91
$schema?: string;
$id?: string;
description?: string;
additionalProperties?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the form of https://github.com/JupiterOne/data-model/blob/178d8838d6004a45ff97b229fd8a0dafd3b50e6f/src/index.ts#L6:

export type IntegrationEntitySchema = {
  $ref?: string;
  allOf?: IntegrationEntitySchema[];
  properties?: {
    [propertyName: string]: any;
  };
  required?: string[];
};

Nothing is required there, which makes sense, allowing for various combinations of those properties.

For this GraphObjectSchema, I think we could make description required. I'm confused about what $schema does here in the context of the metadata for an entity. Will $id ever be provided, perhaps we could remove that? We could probably use some documentation on these properties.

What do you think about naming this EntitySchemaExtension?

@ndowmon ndowmon merged commit eeff93e into main Feb 14, 2022
@ndowmon ndowmon deleted the add-schema-to-graph-object-metadata branch February 14, 2022 14:49
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.

None yet

3 participants