Repository navigation
feat(incidents): add incident support for schemaField - #19115
AdrianMachado merged 4 commits into
Conversation
github-actions
Bot
added
product
Bundle ReportBundle size has no change ✅ |
Danishlynx
left a comment
Danishlynx
left a comment
There was a problem hiding this comment.
Reviewed against what we just did on #19112, since the shape is deliberately the same. The diff matches the verified pattern: the registry entry sits correctly in the schemaField block, SchemaFieldEntity is the right extend target and the right entitiesWithIncidents name (good catch on the decoy, your graphql-js verification table is a nice touch), and skipping PoliciesConfig is right per the #18999 discussion, since there is nothing to extend and columns are governed through their dataset.
One thing worth fixing before undrafting: the docstrings in the new SDL block carry the stale wording the bot review caught on my PR, count described as a start offset and stage and priority as "defaults to any state". I corrected those on the ML blocks in bd45ffc, so suggest the same three lines here so the new block does not reintroduce it: stage "defaults to any stage", priority "defaults to any priority", count "Optional number of results to return, defaults to 20". The five old blocks still carry the stale text, that cleanup can stay a separate change.
On tests: #19112 now parameterizes EntityIncidentsResolverTest on the source entity (assertIncidentsResolvedForSource). Once it merges, a SchemaFieldEntity case is about six lines if a reviewer wants one.
On the entitiesWithIncidents conflict, agreed it is trivial and additive. Happy to rebase whichever of us merges second.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Triage of the one red check, since it does not touch this change. Playwright E2E (1, 8) failed in the The other seven shards built the same images from the same commit and passed. Everything that exercises the change is green: This was already Attempt #2, so a rerun of that single shard should clear it if someone with access can trigger one. Same failure mode @Danishlynx triaged on #19112 earlier today. Still a draft pending your call on authorization, @AdrianMachado. |
|
Thanks, and taken on all three. Docstrings: fixed in 20d4a31, matching your corrected wording exactly, so stage reads "defaults to any stage", priority "defaults to any priority", and count "Optional number of results to return, defaults to 20". You are right about how it got there: I copied the Tests: that is the better sequencing. I will add the Conflict: agreed, and since yours is further along, I will rebase this one rather than the other way round. While we are on the incident docstrings, one thing your fix surfaced that may be worth a line in the follow-up: the stale wording came from the five original blocks, and there are now two blocks with corrected wording and five without. Worth cleaning the five in one pass rather than each new entity inheriting the old text and being corrected in review, which is twice now. Still a draft pending @AdrianMachado on the authorization question from #18999. For what it is worth, Adrian already listed |
|
All sounds right, and thanks for the quick turnaround on the docstrings. On the five stale blocks: agreed it should be one pass instead of correcting each new entity in review. Happy to take that as a small docs-only follow-up once this and #19112 have both landed, so it conflicts with neither; at that point your block and my two already carry the corrected text and exactly the five originals remain. If Jonathan would rather also have #19097 nudge new blocks toward the corrected wording, even better, but the one-pass cleanup stands either way. |
|
Ran the six surfaces against master ( schemaField on master
This PR covers the middle three. The two frontend surfaces are untouched, and that is deliberate: Two things the derivation turned up that are outside this PR
Complete across all six surfaces today: One detail on the parse, worth carrying into #19097. The badge alias is declared inside a fragment for Still a draft pending @AdrianMachado on the authorization question from #18999. |
20d4a31 to
68a0705
Compare
|
@cnpierrepapi answering the auth question so this doesn’t stay a draft for that. This PR stays gates 2–4 only. No Auth: leave it. Don’t invent per-column policy resources. After #19112 merges: rebase (you’ll conflict on Don’t stack this on #19112 — different authors, both targeting master. Native GitHub stacks are for one person’s layer chain. |
|
#19112 merged, so this PR is now CONFLICTING (expected: Please rebase onto master, keep gates 2–4 only, and add a |
schemaField has been a valid incident target in the IncidentOn allowlist in IncidentInfo.pdl, but nothing could read those incidents back. The entity had no incidentsSummary aspect, no incidents field in the GraphQL SDL, and no entry in the resolver list in GmsGraphQLEngine, so writes succeeded and every read reported nothing. This closes the read path: - register incidentsSummary for schemaField in entity-registry.yml - extend SchemaFieldEntity with an incidents field in incident.graphql - add SchemaFieldEntity to entitiesWithIncidents in GmsGraphQLEngine - document the capability in the entity docs The GraphQL type is SchemaFieldEntity, not SchemaField. Both types exist: SchemaFieldEntity is the entity, while SchemaField is the struct carried inside schemaMetadata. Extending SchemaField parses and builds a valid schema, but the field would resolve through the default PropertyDataFetcher and return null, since only entities are wired in entitiesWithIncidents. Authorization is deliberately unchanged. There is no SCHEMA_FIELD_PRIVILEGES resource in PoliciesConfig, and column level operations elsewhere authorize against the parent dataset, so no per column policy resource is introduced here. Reading incidents on a field depends on the schemaFieldEntityFetchEnabled feature flag, as with any other field metadata read through GraphQL.
68a0705 to
c884ce7
Compare
AdrianMachado
left a comment
AdrianMachado
left a comment
There was a problem hiding this comment.
Rebase after #19112 looks right, and the resolver test case is in. Approving the code; still a draft — undraft when you want it in the merge queue.
Six gates (schemaField)
| Gate | This PR |
|---|---|
1 Write (IncidentOn) |
already master (#18478) |
| 2 Summary | incidentsSummary on schemaField ✓ |
| 3 SDL | extend type SchemaFieldEntity (not the SchemaField decoy) ✓ |
| 4 Resolver | "SchemaFieldEntity" in entitiesWithIncidents ✓ |
| 5 Badge / 6 Tab | correctly out — no Incidents tab on SchemaFieldEntity today |
builder.type("SchemaFieldEntity", …) will merge with the existing SchemaFieldEntity wiring (~line 2267). Same pattern as ML.
Docstrings match the corrected ML wording (stage/priority/count). Docs call out schemaFieldEntityFetchEnabled. Auth unchanged as agreed — no SCHEMA_FIELD_PRIVILEGES; parent-dataset check stays a follow-up.
CI
Cancelled runs are not this diff. Same infra pattern as #19112: jobs stuck on Install dependencies / Playwright deps (GitHub-hosted), then cancelled (some after ~1h, smoke after ~6h). metadata-io, build & test, codegen, and several integration shards did pass. Re-run failed/cancelled workflows when you undraft; don’t change the PR for it.
Ship after undraft + green (or override the same flakes).
|
Linear: OBS-2087 Thanks for your contribution! We have created an internal ticket to track this PR. A member of the core DataHub team will be assigned to review it within the next few business days - you will get a follow-up comment once a reviewer is assigned. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Your PR has been assigned to @AdrianMachado (adrian.machado) for review (OBS-2087). |
maggiehays
added
the
merge-pending-ci
8c82366
into
datahub-project:master
Closes the
schemaFieldhalf of the incident entity drift tracked in #18999. Companion to #19112, which doesmlModelandmlFeature; @AdrianMachado asked forschemaFieldto be split into its own PR so it does not block the ML work, and @Danishlynx confirmed on #18999 that it is mine to take.The gap
schemaFieldis already a valid incident target. It is in theIncidentOnallowlist inIncidentInfo.pdl, soraiseIncidentaccepts it and the incident is stored. Nothing could read those incidents back:IncidentOnallowlist (write)incidentsSummaryaspectincidentsfield in GraphQL SDLentitiesWithIncidentsresolver listThe write succeeded and every read reported no incidents.
The type is
SchemaFieldEntity, and this one has a decoyentity.graphqldeclares two similarly named types:extend type SchemaField { incidents ... }is not a syntax error and not a build error. It produces a valid schema with anincidentsfield, so an SDL level check passes. ButSchemaFieldimplements no interfaces and is not an entity, so it is never wired inentitiesWithIncidents, the field resolves through the defaultPropertyDataFetcher, and it returns null with no error.I checked this rather than assuming it. Assembling all 35 SDL files the way
GmsGraphQLEnginedoes and building the schema with graphql-js:The third row is the failure mode, and it builds cleanly.
SchemaFieldEntityreportsimplements: EntityWithRelationships, Entity, HasLogicalParent;SchemaFieldreportsimplements: (none).@JonathanSolvesProblems this may be worth a line in #19097. The SDL set that check derives is a set of type names, and for this entity one of the plausible names is a decoy that would satisfy the check while resolving nothing.
Authorization is deliberately unchanged
There is no
SCHEMA_FIELD_PRIVILEGESinPoliciesConfig, and unlike #19112 there is no existing shape to extend:grep SCHEMA_FIELDoverPoliciesConfig.javareturns nothing on master. Introducing one would mean inventing per column policy resources, which is a product decision rather than a gap fix.Per @Danishlynx on #18999, that is also the wrong direction. Column level operations elsewhere already authorize against the parent dataset, for example
UpdateDescriptionResolver.updateDatasetSchemaFieldDescriptionpassing the dataset urn intoDescriptionUtils.isAuthorizedToUpdateFieldDescription. Columns are governed through their dataset. So this PR changes no authorization at all, and the natural follow up, if maintainers want dataset scoped policies to bind for field incidents, is for the incident resolvers to run the existingEDIT_ENTITY_INCIDENTScheck against the parent dataset urn embedded in the schemaField urn. Happy to do that here or as a follow up, whichever you prefer.Scale
Registering
incidentsSummaryforschemaFielddoes not materialize an aspect on every column.IncidentsSummaryHookiteratesincidentInfo.getEntities()and writes only to the urns an incident actually references, so cost scales with fields that have incidents, not with the number of columns. The registry entry itself is free.Feature flag
Reading incidents on a field goes through the same path as any other field metadata read in GraphQL, so it depends on
schemaFieldEntityFetchEnabled. Raising and resolving are unaffected. This is stated in the entity docs rather than left for people to discover.What I verified, and what I did not
Verified:
SchemaFieldEntity.incidentsbinds toEntityIncidentsResultwith the expected arguments, shown against master as a baselineIncidentsSummaryHookwrites only to referenced urns, read from the hook sourcedatahub-web-react/.prettierrc.jsNot verified, and I would rather say so than imply otherwise:
ImmutableList, and the SDL block is a structural copy of the existingChartblock, but CI is the first real compile.EntityIncidentsResolveris entity agnostic and takes the urn from the source entity, so I do not think a new resolver test earns its place, but if you want one in the shape Adrian described for PR1 I will add it.Leaving this as a draft until @AdrianMachado confirms the authorization call, since that is the only decision here that is not mine to make.
Note for whoever merges second: this and #19112 both edit the
entitiesWithIncidentsline, so one of them will need a trivial conflict resolution. There is no logical conflict, both are additive.Checklist
Summary by cubic
Adds incident read support for schema fields. Previously, incidents could be raised on
schemaFieldbut GraphQL reads returned none.incidentsSummaryforschemaFieldinentity-registry.yml.SchemaFieldEntitywithincidents(...)inincident.graphqland adds it toentitiesWithIncidentsresolvers inGmsGraphQLEngine(target the entity typeSchemaFieldEntity, not theSchemaFieldstruct).schemaFielddocs and adds resolver test coverage.Notes
schemaFieldEntityFetchEnabledflag; raising/resolving incidents is unchanged.Written for commit ffdefc6. Summary will update on new commits.