Skip to content

DEV: Ensure that Serializers forward their scopes, now and forever #32984

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mdoggydog
Copy link
Contributor

This commit is based on the premise that every serializer should have a scope parameter which is an instance of a Guardian.

In many places, serializers which construct other serializers neglect to forward their scope parameter to the sub-serializer. When one of these sub-serializers is later modified to make use of its scope in a new way or under a new circumstance, the missing scope along certain code paths can cause failures which are hard to debug. It is difficult to pin down where, within a chain of serializers, the scope has not been forwarded.

This commit fixes all the current call-sites (in code with test coverage), where a serializer has neglected to forward its scope. It also introduces a mechanism to track where other classes fail to provide a scope when constructing a serializer. This mechanism will cause the codebase to evolve to a state where all serializers are reliably and predictably provided with a Guardian as their scope.

Elements of this commit:

  1. Modify ApplicationSerializer#new to log/raise an error if a serializer is constructed without a scope: argument. (Error is raised only in a test environment.)

  2. Add a scope: scope argument to every call-site where a serializer is constructed by another serializer (forwarding sites) and the scope: argument is missing.

  3. Define PlaceholderGuardian, to be used to provide a scope: argument wherever one was missing in a non-forwarding serializer construction.

  4. Add a new scope: PlaceholderGuardian.new argument to every remaining (non-forwarding) call-site where a serializer is constructed without a scope: argument. (Call-sites which already have a scope: argument have been left untouched, even if it assigned the scope to nil.)

@github-actions github-actions bot added the chat PRs which include a change to Chat plugin label May 28, 2025
@mdoggydog
Copy link
Contributor Author

Regarding the failing tests:

  • I purposefully am allowing the linting tests to fail for the moment; the failures all involve line-wrapping --- but leaving the code as-is with long lines (and minimal changes in the diff) makes it much easier to see what the changes are.
  • In spec/requests tests, which make a simulated HTTP call to the backend and check what the backend emits: is there some way to capture/see what logging or error-raising happened in the backend code?
  • In plugins tests: oh, I see the test framework pulls in all the Official™ plugins from separate repos (which is great!). What is a good way to do this in a dev environment? (E.g., can one just do a git clone within the plugins/ directory --- is git ok with a git-repo within a git-repo???)

This commit is based on the premise that every serializer should have
a `scope` parameter which is an instance of a `Guardian`.

In many places, serializers which construct other serializers neglect to
forward their `scope` parameter to the sub-serializer.  When one of these
sub-serializers is later modified to make use of its scope in a new way or
under a new circumstance, the missing scope along certain code paths can cause
failures which are hard to debug.  It is difficult to pin down where, within a
chain of serializers, the scope has not been forwarded.

This commit fixes all the current call-sites (in code with test coverage),
where a serializer has neglected to forward its scope.  It also introduces a
mechanism to track where other classes fail to provide a scope when
constructing a serializer.  This mechanism will cause the codebase to evolve
to a state where all serializers are reliably and predictably provided with
a `Guardian` as their scope.

Elements of this commit:

 1. Modify `ApplicationSerializer#new` to log/raise an error if a serializer
    is constructed without a `scope:` argument.  (Error is raised only in a
    test environment.)

 2. Add a `scope: scope` argument to every call-site where a serializer is
    constructed *by another serializer* (forwarding sites) and the `scope:`
    argument is missing.

 3. Define `PlaceholderGuardian`, to be used to provide a `scope:` argument
    wherever one was missing in a non-forwarding serializer construction.

 4. Add a new `scope: PlaceholderGuardian.new` argument to every remaining
    (non-forwarding) call-site where a serializer is constructed without a
    `scope:` argument.  (Call-sites which already have a `scope:` argument
    have been left untouched, even if it assigned the `scope` to `nil`.)
@mdoggydog mdoggydog force-pushed the maddog-forward-scope-to-all-serializers branch from fa783b7 to e367c19 Compare May 29, 2025 23:35
@mdoggydog
Copy link
Contributor Author

FYI: This PR is being discussed in Disabling "enable names" makes admin act strange - Bug - Discourse Meta

(E.g., can one just do a git clone within the plugins/ directory --- is git ok with a git-repo within a git-repo???)

Answering my own question: I see that .gitignore cleverly ignores everything in plugins/ that is not part of this repo already, so I think, in this case, the answer is "yes".

(I could still use a hint for my other question, regarding spec/requests tests.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chat PRs which include a change to Chat plugin
Development

Successfully merging this pull request may close these issues.

1 participant