Skip to content

Fix valueFromAST variable own-property checks#4652

Merged
yaacovCR merged 3 commits intographql:16.x.xfrom
abishekgiri:fix-value-from-ast-own-property-check
Apr 18, 2026
Merged

Fix valueFromAST variable own-property checks#4652
yaacovCR merged 3 commits intographql:16.x.xfrom
abishekgiri:fix-value-from-ast-own-property-check

Conversation

@abishekgiri
Copy link
Copy Markdown
Contributor

@abishekgiri abishekgiri commented Apr 2, 2026

Summary

Fixes an issue in valueFromAST where own-property checks were not properly enforced, which could lead to incorrect handling of inherited properties.

Changes

  • Updated own-property checks to ensure only direct properties are considered
  • Prevent unintended prototype chain access

Testing

  • Verified behavior manually with edge cases
  • Existing tests pass locally

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 2, 2026

@abishekgiri is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel.

A member of the Team first needs to authorize it.

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Apr 2, 2026

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: abishekgiri / name: Abishek Kumar Giri (702e107)
  • ✅ login: abishekgiri / name: giria660 (89efacc, c620dcf)

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@abishekgiri
Copy link
Copy Markdown
Contributor Author

Status update

  • Fixed valueFromAST() to use own-property checks when deciding whether a variable was provided.
  • Added regression coverage for omitted prototype-colliding names like toString across scalar, list, and input-object cases.
  • Results: targeted mocha, eslint, and prettier checks passed for this change.
  • tsc in this environment still reports existing external errors from ../node_modules/@types/react-dom, which are outside this PR.

Comment thread src/utilities/valueFromAST.ts Outdated
@yaacovCR
Copy link
Copy Markdown
Contributor

@abishekgiri this looks to me like a legitimate bug fix, although seemingly for a longstanding bug. Thanks for this contribution! Made a small suggestion above, and there is a lint/prettier issue.

@abishekgiri
Copy link
Copy Markdown
Contributor Author

@yaacovCR Pushed a follow-up update addressing the review feedback. This keeps the own-property fix for prototype-colliding names, restores the previous behavior for variables explicitly set to undefined, and adds regression tests for those cases. Targeted mocha, eslint, and prettier checks are passing on the updated branch.

@abishekgiri abishekgiri requested a review from yaacovCR April 13, 2026 22:57
@yaacovCR
Copy link
Copy Markdown
Contributor

@abishekgiri

This seems right to me. valueFromAST is deprecated in v17, but it has not been removed, and this seems like an opportunity for an additional PR to forward-port. Let me know if you have time, otherwise I will try to take care of it. In v17 we can use Object.hasOwn, but otherwise it should be pretty similar.

@abishekgiri
Copy link
Copy Markdown
Contributor Author

@yaacovCR Thanks, that sounds good to me. I’m glad the 16.x.xfix looks right as-is. I also checked the current17.x.xbranch and the samevalueFromAST own-property issue still appears to be present there, so a small forward-port PR makes sense. I can put that together if helpful; otherwise I’m also happy for you to handle that follow-up.

@yaacovCR
Copy link
Copy Markdown
Contributor

Check was apparently dropped back in #553 !

@yaacovCR yaacovCR merged commit db2987c into graphql:16.x.x Apr 18, 2026
19 of 20 checks passed
@yaacovCR yaacovCR added the PR: bug fix 🐞 requires increase of "patch" version number label Apr 18, 2026
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Apr 18, 2026
yaacovCR added a commit that referenced this pull request Apr 18, 2026
forward port of #4652

Co-authored-by: Abishek Kumar Giri <giriabishekkumar5@gmail.com>
@yaacovCR
Copy link
Copy Markdown
Contributor

forward-ported in #4671 => thanks @abishekgiri !!!

@abishekgiri abishekgiri deleted the fix-value-from-ast-own-property-check branch April 19, 2026 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: bug fix 🐞 requires increase of "patch" version number

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants