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

Mixins need to enforce read-only behavior for default instance #61

Open
skybrian opened this issue Jul 22, 2016 · 2 comments
Open

Mixins need to enforce read-only behavior for default instance #61

skybrian opened this issue Jul 22, 2016 · 2 comments

Comments

@skybrian
Copy link
Contributor

For each protobuf class there is a default instance, which should be read-only. Mixins may add extra fields and a way to set them. Any mixin with a setter should throw an exception if the instance is read-only.

Currently the _isReadOnly getter is private. We should make it public. In the meantime, a mixin can implement the check like this:

if (this is ReadonlyMessageMixin) {
  throw new UnsupportedError(
    "attempted to set <somefield> on a read-only message (${info_.messageName})");
}
@osa1
Copy link
Member

osa1 commented May 10, 2022

Currently the _isReadOnly getter is private. We should make it public

We now have a getter for _isReadOnly:

/// Returns `true` if this message is marked read-only. Otherwise `false`.
///
/// Even when `false`, some sub-message could be read-only.
///
/// If `true` all sub-messages are frozen.
bool get isFrozen => _fieldSet._isReadOnly;

I don't understand the story with mixins enough to comment on whether the problem with not enforcing read-only behavior in mixins still exist..

EDIT: Just learned that PbMapMixin is to provide a map-like interface for messages.

@skybrian
Copy link
Contributor Author

At the time I added mixins, there were only a few, either in the open source code or in the google3 version. If all the mixins handle readonly protobufs correctly then there is no issue. It might be good to update them to call isFrozen?

I think this bug was to document the workaround and to warn about what might go wrong if the ability to add mixins was used more widely. (For example, users started writing their own mixins.)

It probably could be closed. If someone were to write a guide to implementing protobuf mixins then it should have a reminder to handle the frozen case correctly, but it doesn't seem too likely?

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

No branches or pull requests

2 participants