Stop the GitHub directory connector storing absent profile fields as the text "None" - #2415
Merged
Merged
Conversation
…the text "None" `email`, `name`, `company` and `role` were built with a Jinja `or none` while carrying `value_type: string`. A rendered `none` becomes the four-character text `None`, so a field the API does not expose is stored as a VALUE rather than as an absence. It matters most for `email`, because identity resolution anchors on it: members whose address the API does not expose all share one "address", group into a single person, and a sign-in can then load a different person's profile (#2393). `''` rather than a real NULL, and deliberately so. `fields_history` detects changes with `curr != prev`, which is NULL-unsafe in ClickHouse: across a NULL boundary the comparison yields NULL and NO history row is emitted in EITHER direction, so a field that later gained a value would never be observed at all — trading a wrong merge for a silent loss. The empty string compares normally, and the downstream filters (`new_value != ''`, `WHERE <field> != ''`) already read it as absence. Two fields in this manifest never had the defect, and both show the shape: `login_normalized` puts `or ''` before its filter, and `member_id` carries no `value_type`, so its `none` stays a real null. The stream tests set `email: None` in the shared member fixture but asserted nothing about it, so the case looked covered and was not. It is now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe GitHub directory connector now converts missing organization member profile fields to empty strings. A test verifies normalization for null ChangesGitHub member profile normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mozhaev-dev
marked this pull request as ready for review
August 11, 2026 03:52
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ingestion/connectors/git/github-directory/tests/test_org_members.py`:
- Around line 141-156: Remove the multi-line explanatory docstring from
test_absent_profile_fields_are_empty_not_the_text_none, leaving the test name
and assertions as the sole expression of the expected behavior.
- Around line 160-169: Update the _member fixture in this test to pass
role=None, and include "role" in the fields asserted by the normalization loop.
Preserve the existing expectation that every nullable field is normalized to an
empty string.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 238f03be-3576-44c0-b793-8184574c9e39
📒 Files selected for processing (2)
src/ingestion/connectors/git/github-directory/connector.yamlsrc/ingestion/connectors/git/github-directory/tests/test_org_members.py
Comment on lines
+141
to
+156
| """A field GitHub does not expose must arrive as `''`, never as `"None"`. | ||
|
|
||
| GitHub returns `null` for a member's e-mail unless it is verified and | ||
| visible to the token's scopes, so this is the COMMON case, not an edge one. | ||
| These fields carry `value_type: string`, which renders a Jinja `none` as the | ||
| four-character text `None` — a value, not an absence. Identity anchors on | ||
| e-mail, so every such member would share one "address" and be grouped into a | ||
| single person: one member's sign-in would then load another's profile. | ||
|
|
||
| Asserted as `''` rather than as a real NULL deliberately. | ||
| `fields_history` detects changes with `curr != prev`, which yields NULL | ||
| across a NULL boundary in ClickHouse and emits NO history row in either | ||
| direction — so a field that later gained a value would never be observed at | ||
| all. `''` compares normally and the downstream filters already read it as | ||
| absence. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the explanatory docstring.
test_absent_profile_fields_are_empty_not_the_text_none already states the behavioral rule. The docstring repeats the rule and implementation details.
As per coding guidelines, “Test non-obvious semantics with a test whose name states the rule rather than adding a comment,” and “Add comments only when code cannot express the reason ... keep them to one line.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/connectors/git/github-directory/tests/test_org_members.py`
around lines 141 - 156, Remove the multi-line explanatory docstring from
test_absent_profile_fields_are_empty_not_the_text_none, leaving the test name
and assertions as the sole expression of the expected behavior.
Source: Coding guidelines
Comment on lines
+160
to
+169
| _page([_member("dev-one", 7001, email=None, name=None, company=None)]), | ||
| ) | ||
|
|
||
| record = read_stream(CONNECTOR, _STREAM, config).records[0].record.data | ||
|
|
||
| for field in ("email", "name", "company"): | ||
| assert record[field] == "", ( | ||
| f"{field} came through as {record[field]!r}; anything other than '' " | ||
| f"is stored as a value and groups unrelated members together" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test null role normalization.
The connector normalizes role to "", but this fixture keeps role="MEMBER" and the assertion loop omits role. Pass role=None and include role in the asserted fields.
Proposed test update
-def _member(login: str, database_id: int, role: str = "MEMBER", **node) -> dict:
+def _member(login: str, database_id: int, role: str | None = "MEMBER", **node) -> dict:
...
- _page([_member("dev-one", 7001, email=None, name=None, company=None)]),
+ _page(
+ [_member("dev-one", 7001, role=None, email=None, name=None, company=None)]
+ ),
...
- for field in ("email", "name", "company"):
+ for field in ("email", "name", "company", "role"):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/connectors/git/github-directory/tests/test_org_members.py`
around lines 160 - 169, Update the _member fixture in this test to pass
role=None, and include "role" in the fields asserted by the normalization loop.
Preserve the existing expectation that every nullable field is normalized to an
empty string.
cyberantonz
approved these changes
Aug 11, 2026
ktursunov
approved these changes
Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2393.
Problem
The GitHub directory manifest builds four profile fields with a Jinja
or nonewhile declaringvalue_type: string:A rendered
nonebecomes the four-character textNone. So a field the GitHub API does not expose is stored as a value, not as an absence.For
emailthat is not cosmetic. Identity resolution anchors on e-mail: the seed groups accounts that share an address into one person. GitHub returnsnullfor a member's e-mail unless it is verified and visible to the token's scopes — the common case, not an edge case — so every such member carries the same"None""address", they group into a single person, and a sign-in through that IdP can resolve to somebody else's profile. That is the failure #2393 describes.The same
or noneshape was onname,companyandrole, so those fields could read as the literalNonetoo.Why
''and not a real NULLDeliberate, and the more interesting half of the change.
fields_historydetects a change withcurr != prev. That comparison is NULL-unsafe in ClickHouse: across a NULL boundary it yields NULL, so no history row is emitted in either direction. Verified:NULL != 'None''x' != NULL'' != 'None''x' != ''With NULL, a field that later gained a value would never be observed at all — trading a wrong merge for a silent loss. The empty string compares normally, and the downstream filters (
new_value != ''inidentity_inputs_from_history,WHERE <field> != ''infields_history) already read it as absence.Two fields in this same manifest never had the defect and both show the intended shape:
login_normalizedputsor ''before its filter, andmember_iddeclares novalue_type, so itsnonestays a real null.Affected areas
connectors/git/github-directory/connector.yaml— the four field definitions.connectors/git/github-directory/tests/test_org_members.py— one new case.Nothing else. No dbt models, no other connector, no backend service.
bronze_github_directory.org_membersalready declares all four columnsNullable(String), so no DDL change is needed, and no relation outside this connector reads these fields.How to test
Expected: 9 passed. The new case (
test_absent_profile_fields_are_empty_not_the_text_none) fails on the previous manifest and passes on this one.The shared member fixture already set
email: Nonebut asserted nothing about it, so the case looked covered and was not — that is why this shipped.What this change does NOT do
Reviewers should know two limits, both structural rather than oversights:
It does not repair rows already written. The persons journal is append-only, so bindings a previous run derived from the
"None"address stay in force and continue to read as resolved — they do not fall back into the operator review queue on their own. An environment that already ingested this connector needs a separate data repair.Correcting the connector does not by itself clear the stale observation.
identity_inputs_from_historyfilters empty values rather than emitting a tombstone, so a"None"→""transition produces a history row the macro then drops — nothing supersedes the earlier observation. Clearing it means removing those rows from the identity inputs, not just fixing the source.🤖 Generated with Claude Code
Summary by CodeRabbit