removes passing reference paths in nested field metadata

[Bangertm]

field2property now ignores a ref key in field metadata and relies
only on the field autoreferencing capability

Self referencing instances of marshmallow.fields.Nested that are
unbound cannot be passed to field2property, which means that the
schema argument of OpenAPI.schema2jsonschema and
OpenAPI.schema2parameters must be a schema instance and not a
schema class

[Bangertm]

This change is to prevent a schema like SelfReferencingSchema passed as a query string parameter from throwing an error due to unbound nested self referencing fields. It is not valid to have a self referencing schema in that location, but at least it doesn't throw an error.

[lafrech]

LGTM. Thanks.

[lafrech]

Some tests fail. From a quick look, it is due to an undeterministic order. I relaunched py37-marshmallow2 after the failure and it passed.

I don't understand how it is related to this change and why it used to work, I don't have much time to look at it right now, but I think the order doesn't matter and the test could be modified to not rely on the order.

[lafrech]

Hint: I think making PetSchema ordered would solve this.

class PetSchema(Schema):

    class Meta:
        ordered = True

I'd still be interested to understand what changed in this PR that triggered this.

[Bangertm]

I think it has to be the change from using the schema class to using an instance that I referenced above. Not sure what the mechanism would be though.

I could just back out that change. The end result of having nested fields in a query string location is invalid by definition. So we could (should?) just raise an error if an invalid field is in a query location. We already kind of do that with many=True:

apispec/apispec/ext/marshmallow/openapi.py

Lines 522 to 523 in f12ca72

	assert not getattr(schema, 'many', False), \
	"Schemas with many=True are only supported for 'json' location (aka 'in: body')"

We can just add a check for invalid fields in a query location such as Nested, List and Dict.

[lafrech]

I think you can have a Nested field in a query location if you use a custom parser such as the NestedQueryFlaskParser here.

Or maybe I'm confused.

[Bangertm]

This looks like a change between OpenAPI v2 and v3. In v2 only primitive types are supported:

Query parameters only support primitive types. You can have an array, but the items must be a primitive value type. Objects are not supported.

In v3 they can be objects:

Query parameters can be primitive values, arrays and objects.

In which case, nesting is perfectly fine. Sorting this out is probably going to take a little more thought.

I noticed that other tests are using res.sort(key=lambda param: param['name'])

apispec/tests/test_openapi.py

Lines 500 to 512 in f12ca72

	def test_schema_query_instance(self, openapi):
	class UserSchema(Schema):
	name = fields.Str()
	email = fields.Email()
	res = openapi.schema2parameters(UserSchema(), default_in='query')
	assert len(res) == 2
	res.sort(key=lambda param: param['name'])
	assert res[0]['name'] == 'email'
	assert res[0]['in'] == 'query'
	assert res[1]['name'] == 'name'
	assert res[1]['in'] == 'query'

So I can probably do that too.

[lafrech]

TBH, I never even wondered if this was OpenAPI compliant... Shame on me. Let's be permissive and don't force the user to be compliant, especially if something is acceptable in v3 and not in v2.

res.sort(key=lambda param: param['name'])

is fine as well. More explicit than the Meta approach.

[Bangertm]

Forgot to look at the failing test before commenting just instancing PetSchema on as in input to schema2parameters on the expected value is good enough to eliminate the problem.

I think there is something not quite right about the nested query string parameters are handled, but it's also likely to be a relatively uncommon use case.

[lafrech]

OK, I don't understand why passing an instance solves the problem but as long as it does...

Should we specify in OpenAPI.schema2jsonschema and OpenAPI.schema2parameters that only instances are accepted?

[sloria]

Should we specify in OpenAPI.schema2jsonschema and OpenAPI.schema2parameters that only instances are accepted?

Looks like @Bangertm already did that for schema2jsonschema.

:param Schema schema: A marshmallow Schema instance

Would be good to document that for schema2parameters as well, but it's not a blocker to merging.

[sloria]

@Bangertm @lafrech All issues and PRs for 1.0 have been resolved! 🎉 I think we're good to release. Unless there are any objections, I'll plan to release 1.0 in the next day or two. 🚢 🇮🇹

[lafrech]

Great. I'm also eager to get this released.

I couldn't help but proposing a last-minute breaking change: #381.

[lafrech]

Guys, I've been trying the latest apispec revision on a huge project of ours to identify potential issues before we release.

I've been spending a great deal of time diving into (my own) legacy spaghetti code dealing with pathological cases of auto-generated inheritance schemas that would generate all kinds of errors and warnings. The code is now a mess, but I think I'm done with the test and I got it to work by simply overriding MarshmallowPlugin.schema_helper and passing a custom schema_name_resolver, which is how this is intended to work.

This means I validate it works in my use case. In fact, the auto-registration feature will allow me to remove a lot of code that will become useless. Great. Thanks again @Bangertm for the work on this.

I wanted to conclude that this is good to go, but there's #383 I didn't have the time to investigate.