Adds warnings to jobmapping validator #3155
Conversation
alishakawaguchi
commented
Jan 16, 2025
•
alishakawaguchi
commented
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3155 +/- ##
==========================================
+ Coverage 29.70% 30.25% +0.54%
==========================================
Files 372 374 +2
Lines 42926 42960 +34
==========================================
+ Hits 12753 12996 +243
+ Misses 28671 28459 -212
- Partials 1502 1505 +3 ☔ View full report in Codecov by Sentry. |
backend/pkg/utils/utils.go
Outdated
| @@ -26,3 +27,27 @@ func DedupeSliceOrdered[T comparable](input []T) []T { | |||
| } | |||
| return output | |||
| } | |||
|
|
|||
| func DedupeSlice[T comparable](input []T) []T { | |||
There was a problem hiding this comment.
FWIW, I think we can actualy remove this function entirely and just use slices.Compact
backend/pkg/utils/utils.go
Outdated
| return output | ||
| } | ||
|
|
||
| func CompareSlices(slice1, slice2 []string) bool { |
There was a problem hiding this comment.
I think we can remove this and use slices.Compare
| message ColumnWarning { | ||
| string schema = 1; | ||
| string table = 2; | ||
| string column = 3; | ||
| repeated string warnings = 5; | ||
| } |
There was a problem hiding this comment.
Please add property descriptions (see ColumnError)
| @@ -1765,6 +1766,7 @@ export async function validateJobMapping( | |||
| }); | |||
| }), | |||
| connectionId: connectionId, | |||
| jobSource: jobSource, | |||
There was a problem hiding this comment.
how is the job source different from the connectionId? aren't they the same thing? Could connectionid go away in favor of just the job source?
There was a problem hiding this comment.
I wanted to keep the validate job mapping endpoint flexible. It doesn't require the jobsource. If you just want to validate the mappings then all you need is mappings and connection_id. Like you could call it with the mappings and destination id and it will tell you all the missing tables
| @@ -109,6 +128,9 @@ export default function FormErrorsCard(props: Props): ReactElement { | |||
| function formErrorsToMessages(errors: FormError[]): string[] { | |||
There was a problem hiding this comment.
I'm very performance sensitive about things changing on the schema table page.
There are potentially a lot of form errors that can occur on this page...
It might be more efficient here to have a formErrorsToMessages function return an object that splits them.
interface FormErrorMesageResponse {
errors: string[];
warnings: string[];
}
Then loop through them and check the level.
That way you reduce the algorithm from O(2N) to just O(N)
| const values = form.getValues(); | ||
| const connection = connections.find((c) => c.id === values.sourceId); | ||
| const job = data?.job; | ||
| try { | ||
| setIsValidatingMappings(true); | ||
| let jobsource: JobSource | undefined; | ||
| if (job && connection) { | ||
| jobsource = create(JobSourceSchema, { | ||
| options: toJobSourceOptions(values, job, connection, values.sourceId), | ||
| }); | ||
| } |
There was a problem hiding this comment.
seems liek this logic is pretty similar to what is above in the validateMappings function, but it's not using the same conditional logic. ... should it be? they look pretty darn similar.
| @@ -208,7 +207,7 @@ func (j *JobMappingsValidator) ValidateCircularDependencies( | |||
| } | |||
|
|
|||
| for table, deps := range validForeignKeyDependencies { | |||
| validForeignKeyDependencies[table] = utils.DedupeSlice(deps) | |||
| validForeignKeyDependencies[table] = slices.Compact(deps) | |||
