Skip to content
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

BUGFIX: Replace graceful failure of field access on dataobjects #399

Merged
merged 2 commits into from
Aug 11, 2021

Conversation

unclecheese
Copy link

@unclecheese unclecheese commented Aug 10, 2021

This is a tricky one with a lot of approaches for a fix, but this is the only way that doesn't break any APIs.

The issue

versioned-snapshot-admin is broken because it queries for fields like absoluteLink and version on the origin record of all snapshots. Sometimes, these origins don't have those fields. The most common case is "SnapshotEvent", which is basically an arbitrary point in time -- "imported translations", e.g. , and these fields don't exist.

In < 3.5, this failed gracefully, because the accessor was a simple $obj->obj($fieldName). In 3.5, we've introduced the FieldAccessor service as a new layer of abstraction. The service we use (for graphql 4 compat) is CaseInsensitiveFieldAccessor . This implementation throws when a field is not found.

This is a regression. Missing fields used to fail gracefully. Now they throw.

Option A

Remove exception from CaseInsensitiveFieldAccessor (API change)

Option B

Add option to suppress errors to CaseInsensitiveFieldAccessor (complicated, and probably an API change if it's enabled by default)

Option C

Update SnapshotEvent to include stubs for AbsoluteLink, Version, etc., that just return null. (Coding to the implementation, not the intention)

Option D

✅ Catch the exception and fail gracefully (Implementation-specific code, as the FieldAccessorInterface does not say the method should throw)

The chosen option D isn't great, but it doesn't break any APIs, should qualify for patch release as it fixes a regression, and it's extremely unlikely that anyone would be using a custom FieldAccessorInterface abstraction.

Criticality

It's blocking upgrades to 4.8. We need this fix to be in a 3.5.x patch release.

@unclecheese unclecheese requested a review from emteknetnz August 10, 2021 22:44
@unclecheese unclecheese force-pushed the pulls/3.5/graceful-accessor branch from e684b7b to c58e241 Compare August 10, 2021 22:46
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge on green

@unclecheese unclecheese merged commit 90570dc into silverstripe:3.5 Aug 11, 2021
@unclecheese unclecheese deleted the pulls/3.5/graceful-accessor branch August 11, 2021 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants