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.

Sign up for GitHub

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
Merged

Add optional schema property to graph object metadata #611

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43496b6
Add optional `schema` property to graph object metadata
ndowmon Feb 10, 2022
e292f84
Update GraphObjectSchema docstrings
ndowmon Feb 10, 2022
9241e8e
Merge branch 'main' into add-schema-to-graph-object-metadata
ndowmon Feb 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@ and this project adheres to

### 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
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.

#612

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

documentation about the properties that an entity / relationship is
expected to have
- Added `executeStepWithDependencies` utility to
`@jupiterone/integration-sdk-testing` package. This allows developers to test
specific integration steps in isolation, while assuring that all of its
Expand Down
17 changes: 16 additions & 1 deletion packages/integration-sdk-core/src/types/step.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { RelationshipClass } from '@jupiterone/data-model';
import {
RelationshipClass,
IntegrationEntitySchema,
} from '@jupiterone/data-model';

import {
ExecutionContext,
Expand Down Expand Up @@ -81,9 +84,21 @@ export interface GraphObjectIndexMetadata {
enabled: boolean;
}

export interface GraphObjectSchema extends IntegrationEntitySchema {
$schema?: string;
$id?: string;
description?: string;
additionalProperties?: boolean;
Comment on lines +88 to +91
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.

Tracking:

}

export interface StepGraphObjectMetadata {
_type: string;

/**
* The schema used to describe the properties assigned to this entity
ndowmon marked this conversation as resolved.
Show resolved Hide resolved
*/
schema?: GraphObjectSchema;

/**
* Indicates the set of data represented by this `_type` should always be
* considered a partial dataset.
Expand Down
2 changes: 1 addition & 1 deletion packages/integration-sdk-testing/src/__tests__/jest.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
createMappedRelationship,
Entity,
ExplicitRelationship,
GraphObjectSchema,
IntegrationInvocationConfig,
IntegrationSpecConfig,
RelationshipClass,
Expand All @@ -11,7 +12,6 @@ import {
toMatchGraphObjectSchema,
toMatchDirectRelationshipSchema,
toTargetEntities,
GraphObjectSchema,
registerMatchers,
toImplementSpec,
} from '../jest';
Expand Down
8 changes: 1 addition & 7 deletions packages/integration-sdk-testing/src/jest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as deepmerge from 'deepmerge';
import {
Entity,
ExplicitRelationship,
GraphObjectSchema,
IntegrationInstanceConfig,
IntegrationInvocationConfig,
IntegrationSpecConfig,
Expand Down Expand Up @@ -222,13 +223,6 @@ function graphObjectClassToSchemaRef(_class: string) {
return `#${_class}`;
}

export interface GraphObjectSchema extends dataModel.IntegrationEntitySchema {
$schema?: string;
$id?: string;
description?: string;
additionalProperties?: boolean;
}

export interface ToMatchGraphObjectSchemaParams {
/**
* The JupiterOne hierarchy class or classes from the data model that will be used to generate a new schema to be validated against. See: https://github.com/JupiterOne/data-model/tree/main/src/schemas
Expand Down