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

dump_only attributes passed in Meta options are not marked as readOnly #84

Closed
sloria opened this issue Jul 19, 2016 · 5 comments

Comments

@sloria
Copy link
Member

commented Jul 19, 2016

Reported by @lafrech in #81 (comment):


I noticed this only adds readOnly if the field was provided dump_only attribute directly. If the field appears in the dump_only list in Meta, readOnly is not added.

Is this known/intended?

More precisely, it "does not work" if spec.definition is passed a marshmallow.schema.SchemaMeta but it "works" if it is passed a Schema instance:

class GistSchema(ma.Schema):
id = ma.fields.Int()
content = ma.fields.Str()

class Meta:
    dump_only = ('content',)

readOnly not added

spec.definition('Gist', schema=GistSchema)

readOnly added

spec.definition('Gist', schema=GistSchema())
I just begun experimenting with apispec and I'm not sure passing an instance is the right thing to do. In the docs, a SchemaMeta is passed.

@sloria sloria changed the title `dump_only` attributes passed in Meta options are not marked as readOnly dump_only attributes passed in Meta options are not marked as readOnly Jul 19, 2016

@sloria

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2017

The workaround for this is to pass a schema instance rather than a class:

spec.definition('Gist', schema=GistSchema())

because the dump_only attribute gets set on the fields upon schema instantiation.

@lafrech

This comment has been minimized.

Copy link
Member

commented Jan 16, 2017

Yes.

I had a look at this issue when it was submitted and didn't find any obvious solution.

This workaroud works but it breaks schema resolving in marshmallow extension. The mechanism relies on Schema classes being stored, not instances.

I would like to suggest the following change to schema_definition_helper in marshmallow extension, so that

  • the schema instance is used to create the json description (this workaround)
  • the schema class is stored (needed for further reference resolving)
def schema_definition_helper(spec, name, schema, **kwargs):
    """Definition helper that allows using a marshmallow
    :class:`Schema <marshmallow.Schema>` to provide OpenAPI
    metadata.

    :param type schema: A marshmallow Schema class or instance.
    """

    if isinstance(schema, type):
        schema_cls = schema
        schema_instance = schema()
    else:
        schema_cls = type(schema)
        schema_instance = schema

    # Store registered refs, keyed by Schema class
    plug = spec.plugins[NAME]
    if 'refs' not in plug:
        plug['refs'] = {}
    plug['refs'][schema_cls] = name
    return swagger.schema2jsonschema(schema_instance, spec=spec, name=name)

Allowing an instance to be passed to the definition helper is not related to the workaround and could be done anyway.

What do you think, @sloria?

@lafrech

This comment has been minimized.

Copy link
Member

commented Jan 16, 2017

On second thought, I'm mixing two issues here.

I just sent a PR to let schema_definition_helper pass schema2jsonschema a Schema instance rather than a class.

The tests fail because this change breaks the use case where schema_definition_helper is passed a Schema instance.

AFAIU, there is an inconsistency between the tests and the docs / comments in the code. The docs say we should pass a Schema class, but we test that both a class and an instance can be passed.

If we want to be able to pass both, then the comments/docs should be updated. And my code proposal above may be relevant.

@sloria

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2017

@lafrech Thank you for looking into this. I think you are correct re: the docs/comments being incorrect. Users should be able to pass either a class or instance, and I think your solution in your previous comment should work.

@lafrech

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

Thank you for the feedback @sloria. I sent a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.