-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Cases] Migrate connector ID to references #104221
[Cases] Migrate connector ID to references #104221
Conversation
* The external service fields. Exporting here for use in the service transformation code so I can define | ||
* a type without the connector_id field. | ||
*/ | ||
export const CaseExternalServiceBasicRt = rt.type({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exporting here for use in the service transformation code so I can define a type without the connector_id field.
rt.intersection([ | ||
CaseExternalServiceBasicRt, | ||
rt.type({ | ||
pushed_at: rt.string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved these up into the basic definition above.
export type ESCasePatchRequest = Omit<CasePatchRequest, 'connector'> & { | ||
connector?: ESCaseConnector; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have been moved into the cases service because they are isolated to the service layer. Other parts of cases no longer need to reference the types. Instead they will always reference the CaseAttributes
or equivalent type.
export type ESCasesConfigureAttributes = Omit<CasesConfigureAttributes, 'connector'> & { | ||
connector: ESCaseConnector; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been moved into the configure service because it is isolated to the service layer. Other parts of the cases plugin no longer need to reference the type. Instead they will reference the CasesConfigureAttributes type directly.
name: string; | ||
type: ESCaseConnectorTypes; | ||
fields: ESConnectorFields | null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were moved into the service layer as well since they are only referenced by service layer functionality.
} catch (error) { | ||
this.log.debug(`Error on UPDATE case configuration ${configurationId}: ${error}`); | ||
throw error; | ||
} | ||
} | ||
} | ||
|
||
function transformUpdateResponseToExternalModel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 : Configuration saved object transformation logic
@@ -0,0 +1,199 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file holds test utility functions.
@@ -0,0 +1,100 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 : Shared transformation logic between the case and configuration saved objects
field: UserActionFieldType | ||
) => { | ||
return field === 'connector' && so.attributes.connector | ||
? transformESConnectorToCaseConnector(so.attributes.connector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we no longer need to do the transformation, we no longer need the abstraction for getField
both can just use get(so, ['attributes', field])
directly as done above.
@@ -605,6 +609,74 @@ export const getConnectorMappingsFromES = async ({ es }: { es: KibanaClient }) = | |||
return mappings; | |||
}; | |||
|
|||
type ESConnectorFields = Array<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These make the integration test types easier. Since these types are no longer defined in the cases common area we need to define them here so we can access them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is ok to import them from x-pack/plugins/cases/server/services/cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I'll give that a shot, I think it was giving me an error, I suppose for this we could just ignore the eslint. Good point.
const { connector, external_service, ...restAttributes } = caseAttributes; | ||
|
||
let transformedAttributes: Partial<ESCaseAttributes> = { ...restAttributes }; | ||
let pushConnectorId: string | undefined | null; | ||
|
||
if (external_service) { | ||
let restExternalService: ExternalServicesWithoutConnectorId | null | undefined; | ||
({ connector_id: pushConnectorId, ...restExternalService } = external_service); | ||
transformedAttributes = { | ||
...transformedAttributes, | ||
external_service: restExternalService, | ||
}; | ||
} else if (external_service === null) { | ||
transformedAttributes = { ...transformedAttributes, external_service: null }; | ||
} | ||
|
||
if (connector) { | ||
transformedAttributes = { | ||
...transformedAttributes, | ||
connector: { | ||
name: connector.name, | ||
type: connector.type, | ||
fields: transformFieldsToESModel(connector), | ||
}, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot think of a way to remove the casting (restExternalService as ExternalServicesWithoutConnectorId
). TS won this fight 😄 .
const { connector, external_service, ...restAttributes } = caseAttributes; | |
let transformedAttributes: Partial<ESCaseAttributes> = { ...restAttributes }; | |
let pushConnectorId: string | undefined | null; | |
if (external_service) { | |
let restExternalService: ExternalServicesWithoutConnectorId | null | undefined; | |
({ connector_id: pushConnectorId, ...restExternalService } = external_service); | |
transformedAttributes = { | |
...transformedAttributes, | |
external_service: restExternalService, | |
}; | |
} else if (external_service === null) { | |
transformedAttributes = { ...transformedAttributes, external_service: null }; | |
} | |
if (connector) { | |
transformedAttributes = { | |
...transformedAttributes, | |
connector: { | |
name: connector.name, | |
type: connector.type, | |
fields: transformFieldsToESModel(connector), | |
}, | |
}; | |
} | |
const { connector, external_service, ...restAttributes } = caseAttributes; | |
const { connector_id: pushConnectorId, ...restExternalService } = external_service ?? {}; | |
const transformedAttributes = { | |
...restAttributes, | |
...(external_service != null | |
? { | |
external_service: restExternalService as ExternalServicesWithoutConnectorId, | |
} | |
: { external_service: null }), | |
...(connector != null | |
? { | |
connector: { | |
name: connector.name, | |
type: connector.type, | |
fields: transformFieldsToESModel(connector), | |
}, | |
} | |
: {}), | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, thanks, I took another stab at this let me know what you think.
if (connectorId === noneConnectorId) { | ||
connectorRef.push({ name: connectorIdReferenceName }); | ||
} else if (connectorId) { | ||
connectorRef.push({ | ||
name: connectorIdReferenceName, | ||
ref: { id: connectorId, name: connectorIdReferenceName, type: ACTION_SAVED_OBJECT_TYPE }, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
if (connectorId === noneConnectorId) { | |
connectorRef.push({ name: connectorIdReferenceName }); | |
} else if (connectorId) { | |
connectorRef.push({ | |
name: connectorIdReferenceName, | |
ref: { id: connectorId, name: connectorIdReferenceName, type: ACTION_SAVED_OBJECT_TYPE }, | |
}); | |
} | |
connectorRef.push({ | |
name: connectorIdReferenceName, | |
...(connectorId !== noneConnectorId ? { ref: { id: connectorId, name: connectorIdReferenceName, type: ACTION_SAVED_OBJECT_TYPE }} : {}), | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored this with the introduction of the ConnectorReferenceHandler
. Let me know what you think.
* The name of the saved object reference indicating the action connector ID. This is stored in the Saved Object reference | ||
* field's name property. | ||
*/ | ||
export const connectorIdReferenceName = 'connectorId'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: connectorIdReferenceName
-> CONNECTOR_ID_REFERENCE_NAME
Maybe we could move these variables to a constants.ts
file in x-pack/plugins/cases/server
}) | ||
: { id: thisCase.id, version: thisCase.version }; | ||
}); | ||
const updateCases: UpdateRequestWithOriginalCase[] = query.cases.reduce( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block now zips togethers the original case if it existed and the updated request. This is particularly useful for when we call the patch
request so that we can populate the originalCase
field. If the original case cannot be found within the map we skip the request. This shouldn't even happen though because the code above should handle finding requests that don't exist.
`); | ||
}); | ||
|
||
it('builds references for connector_id, connector.id, and includes the existing references', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for connector_id, connector.id
-> for external service connector id, case connector id
or we can put a comment to explain what are those in the whole file test.
} from './transform'; | ||
|
||
describe('service transform helpers', () => { | ||
describe('findConnectorIdReference', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have two references with the same id?
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
Saved Objects .kibana field count
Unknown metric groupsAPI count
API count missing comments
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job as always!
I created a few connectors on master. Then I switched to your PR to test the migration. I also played around with the UI. I could not break anything 🚀. LGTM!
Friendly reminder: Looks like this PR hasn’t been backported yet. |
* Starting configure migration * Initial refactor of configuration connector id * Additional clean up and tests * Adding some tests * Finishing configure tests * Starting case attributes transformation refactor * adding more tests for the cases service * Adding more functionality and tests for cases migration * Finished unit tests for cases transition * Finished tests and moved types * Cleaning up type names * Fixing types and renaming * Adding more tests directly for the transformations * Fixing tests and renaming some functions * Adding transformation helper tests * Adding migration utility tests and some clean up * Begining logic to remove references when it is the none connector * Fixing merge reference bug * Addressing feedback * Changing test name and creating constants file
Friendly reminder: Looks like this PR hasn’t been backported yet. |
2 similar comments
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
* Starting configure migration * Initial refactor of configuration connector id * Additional clean up and tests * Adding some tests * Finishing configure tests * Starting case attributes transformation refactor * adding more tests for the cases service * Adding more functionality and tests for cases migration * Finished unit tests for cases transition * Finished tests and moved types * Cleaning up type names * Fixing types and renaming * Adding more tests directly for the transformations * Fixing tests and renaming some functions * Adding transformation helper tests * Adding migration utility tests and some clean up * Begining logic to remove references when it is the none connector * Fixing merge reference bug * Addressing feedback * Changing test name and creating constants file
* Starting configure migration * Initial refactor of configuration connector id * Additional clean up and tests * Adding some tests * Finishing configure tests * Starting case attributes transformation refactor * adding more tests for the cases service * Adding more functionality and tests for cases migration * Finished unit tests for cases transition * Finished tests and moved types * Cleaning up type names * Fixing types and renaming * Adding more tests directly for the transformations * Fixing tests and renaming some functions * Adding transformation helper tests * Adding migration utility tests and some clean up * Begining logic to remove references when it is the none connector * Fixing merge reference bug * Addressing feedback * Changing test name and creating constants file
Background: #105677
This PR Migrates the actions connector ID fields we have stored in the Configuration and Case saved objects. We still need to finish user actions, which will be done in a subsequent PR.
The general approach I took was to move all transformations into the service layer. There is now an external model which is returned the UI and used in most places within the backend. The internal (ES) model is used only within the service layer.
Going from the external model to the ES model
Prior to this change we were already transform the
connector.fields
from an object to an array of objects that had a key and value (i.e. for jira{key: 'issueType', value: 'bug'}
)I moved this transformation into the service layer as well.
In addition to the fields transformation I remove the
connector.id
(for the configuration and cases saved objects) and move it to the references array.Going from the ES model to the external model
Prior to this change we already transformed the
connector.fields
from the ES representation of an array of objects I mentioned above into an object with the fields like:{issueType: bug}
to follow the example above.This was also moved to the service layer. So after the saved object client does a get, post, update, find, etc we transform the ES model into the external model by converting the
connector.fields
, finding theid
from the references array and placing it back inconnector.id
andexternal_service.connector_id
.Saved Object Migration
In addition to moving the transformation logic to the service layer we also need to the existing saved objects using a migration. I split out the
configuration
andcases
migrations into their own files since themigrations.ts
file was getting kind of large.The migration removes the
connector.id
for the cases and configuration saved objects and moves it to the references array. Forexternal_service.connector_id
it does the same thing.Errors
Going from the ES model to the external model
If the transformation can't find a reference called
connectorId
inside the references array it will set theconnector
field to the defaultnone
connector (this is what the cases and configuration saved objects should have if no connector is defined.)If the transformation can't find a reference called
pushConnectorId
inside the references array it will set theexternal_service.connector_id
field tonull
. It leaves the rest of the fields inexternal_service
. I did this intentionally so that we wouldn't accidentally get data loss since theexternal_service
saves a good bit of information about the push details.Going from the external model to the ES model
If the
connector.id
orexternal_service.connector_id
are valuenone
the transformation will not create a reference for them. it will still remove the connector id field though. If they are defined but notnone
then it will move the value into the references field.If the
connector.id
is undefined for some reason, we won't create a reference.If the
external_service
is null or theexternal_service.connector_id
is null it won't create a reference.Testing
I tried to add good coverage using unit tests for the migration and transformations. I also added a few integration tests that look in the saved object to ensure that the ids are removed appropriately.