Skip to content

[Python] Add validation to Python client #2794

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

Merged
merged 9 commits into from
May 9, 2016

Conversation

scottrw93
Copy link
Contributor

Python client validation for #2663. Just two questions about this:

One think to note is the regex defined the test spec. This pattern, pattern: /[a-z]/i, will work with the Ruby client allowing strings containing only upper or lower case letters. This will not be the case for the Python client and other clients I think. Should this be made more generic in the example.

Also, the Ruby client supports allowable values on the Api paramters. Does the spec allow you define an enumerator on a parameter in an operation and if so do you have an example? I thought it was only parameters in models that could be an enum.

@wing328
Copy link
Contributor

wing328 commented May 7, 2016

About the regex expression, the example conforms to the spec.

For Python, you will need to override toRegularExpression in PythonClientCodegen.java to convert the pattern into regular expression in Python convention.

For example, /\n/ needs to be converted to /\\n/for Java API client.

(Linked to #2663)

@scottrw93
Copy link
Contributor Author

scottrw93 commented May 9, 2016

Ok, Python does not support modifiers the same way as Perl, Ruby, PHP. See the docs for details. This means the toRegularExpression() method is somewhat redundant. Instead I have used the postProcess methods to map the modifiers to the equivalent Python flag, creating two vendor extensions from the pattern, one being the regex and the other a list of the modifiers.

}

vendorExtensions.put("x-regex", regex);
vendorExtensions.put("x-modifiers", modifiers);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Create two extension from the existing pattern, one the regex the other the modifiers.

@wing328 wing328 merged commit 4718c34 into swagger-api:master May 9, 2016
@zroadhouse-rmn
Copy link

zroadhouse-rmn commented Aug 8, 2016

Question regarding the requirement that patterns must start with a "/". Swagger-codegen understands how to parse (i.e. strip) the leading and trailing slashes. However, other tools in the Python ecosystem do not -- particularly the jsonschema module which is used by other swagger tooling. Can you relax the requirement that the pattern must begin with "/" in PythonClientCodegen.java?

Example Swagger definition (modified to force use of leading/trailing slashes):

definitions:
  DatastoreData:
      engine_name:
        pattern: /^[a-zA-Z0-9_]+$/
        type: string

Generated Python code (strips the leading/trailing slashes):

    @engine_name.setter
    def engine_name(self, engine_name):
        """
        Sets the engine_name of this DatastoreData.


        :param engine_name: The engine_name of this DatastoreData.
        :type: str
        """

        if not engine_name:
            raise ValueError("Invalid value for `engine_name`, must not be `None`")
        if not re.search('^[a-zA-Z0-9_]+$', engine_name):
            raise ValueError("Invalid value for `engine_name`, must be a follow pattern or equal to `/^[a-zA-Z0-9_]+$/`")

        self._engine_name = engine_name

Note: generated using version 2.2.0

@scottrw93
Copy link
Contributor Author

scottrw93 commented Aug 8, 2016

I'm not sure I follow fully. Swagger patterns follow the /pattern/modifiers model. The pattern supplied in the spec file is parsed into the regex expression and the modifiers. So in your example above you can see the actual validation is checking for a string containing only letters of any case and numbers (with no slashes).

pattern: /^[a-z0-9_]+$/i would have produced the same result as it uses the ignore case modifier and would have produced this validation check if not re.search('^[a-z0-9_]+$', engine_name, re.IGNORECASE):

Why does it matter if other Python tools do not understand these slashes? Python does not handle regexes using this pattern so I would not expect them to. The Swagger spec file is only read by Swagger Parser and patterns need to be expressed in a consistent way across all clients in my opinion.

@zroadhouse-rmn
Copy link

I walked the specs and can see where you are coming from:

I have some pre-existing JSON schemas that I'm incorporating into my swagger definition that were not using slashes and were working ok with the jsonschema package (v2.5.1 => https://github.com/Julian/jsonschema) which simply passes the pattern straight through to re.search. So when the swagger-codegen behavior changed in 2.2.0 to require the slashes, I break one tool or the other by making the change. Perhaps this is a better issue for the jsonschema package.

I didn't have any issues with validating the swagger via swagger-editor. Should it also enforce the slashes?

@wing328
Copy link
Contributor

wing328 commented Aug 9, 2016

@zroadhouse-rmn please kindly open an issue with Swagger Editor (https://github.com/swagger-api/swagger-editor) about validating the regular expression to ensure it starts and ends with /

@wing328 wing328 modified the milestones: v2.2.2, v2.2.0 Aug 9, 2016
@zroadhouse-rmn
Copy link

zroadhouse-rmn commented Aug 9, 2016

The documentation for JSON schemas does not show slashes in regex patterns:

I think the use of slashes is more about the language that implements regexs (i.e. a special syntax in JavaScript) than about the patterns themselves. My suggestion would be to implement the parser such that you can support patterns that use them or not.

@scottrw93
Copy link
Contributor Author

That is just an example of how they implement i think, not a hardens rule of how it must be done. Swagger has to support clients in many languages and many of them have different ways of representing regexes. There needs to be a consistent way of representing them in the spec file as otherwise a string that is valid in one client won't be in another client. Swagger just so happens to use the perl/UNIX convention, I presume because it allows you to easily use modifiers as part of your regex and many other languages use it, such as Ruby.

@zroadhouse-rmn
Copy link

I agree around consistent representation being helpful.

It appears to me as a user that requiring the slashes in the pattern field is a backwards incompatible change to the semantic meaning of the "pattern" field. There are likely many swagger specs and JSON schema specs out there that do not include them today (some small sample found via a search "swagger pattern").

The Swagger spec doesn't state this as a difference in Swagger from JSON schema. It references http://json-schema.org/latest/json-schema-validation.html#anchor33. The JSON schema spec discusses the cross-platform/language issue in http://json-schema.org/latest/json-schema-validation.html#anchor6 and also doesn't mention slashes. The JSON schema examples listed above explicitly don't show their use. A search of the OpenAPI-Specification repository shows patterns without slashes (ex: "^https?://", "^/", "^[a-zA-Z0-9_]+$", "^[^{}/ :\\]+(?::\d+)?$").

@wing328
Copy link
Contributor

wing328 commented Aug 12, 2016

In http://www.ecma-international.org/ecma-262/5.1/, the examples of regular expression I found start with "/" but I've not read the whole document to confirm "/" is a must

I'm thinking about checking whether the regular expression starts with "/". If not, just enclose the regular expression string with "/"

@zroadhouse-rmn
Copy link

In section http://www.ecma-international.org/ecma-262/5.1/#sec-7.8.5, it describes Regular Expression Literals. That's a special ECMAScript syntax that uses the slashes are converted to RegExp objects http://www.ecma-international.org/ecma-262/5.1/#sec-15.10.4. The actual pattern is the text in between the slashes but does not include the slashes themselves. This is pretty clear when you look at the RegExp constructor - that the pattern and modifiers are two separate fields.

Based on that, I don't think you can adapt the pattern field in the JSON schema specification (as inherited by Swagger) to mean pattern + modifier and use slashes as that would change the semantic meaning of the field in a backwards incompatible way.

@wing328
Copy link
Contributor

wing328 commented Aug 22, 2016

@zroadhouse-rmn I've submitted #3623 to add the missing delimiter, if any, to the regular expression.

@erikvanzijst
Copy link
Contributor

We actually had this discussion internally here to as to whether or not Swagger requires the use of slashes around the actual pattern and independently came to the same conclusion as @zroadhouse-rmn.

Swagger merely uses json schema, which in turn states the use of ECMA 262, which declares the slashes in its RegularExpressionLiteral.

While it's not entirely clear whether json schema, when referring to "a valid regular expression, according to the ECMA 262", meant ECMA's RegularExpressionBody, or RegularExpressionLiteral, the fact that all of json schema's own examples do not use surrounding slashes in patterns, strongly seems to indicate the former.

Additionally, I have not seen any real world swagger files using slashes.

Json schema could have been a bit more explicit, but the slashes seem more of a syntactic construct for using patterns as literals in javascript, but not actually a part of the regular expression. In contrast to Javascript, JSON encodes patterns in quotes, making the slashes redundant.

One argument in favor of slashes might be that it would provide a means of specifying flags and that does indeed appear to be an issue in json schema.

@wing328 wing328 changed the title Add validation to Python client [Python] Add validation to Python client Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants