-
-
Notifications
You must be signed in to change notification settings - Fork 762
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
Add ability to add random string to endpoint names #275
Conversation
complain about duplicate names.
@jfinkhaeuser thanks for the PR. But could you please explain what is a user case for this PR? |
|
||
def generator(size=randomize, chars=string.ascii_uppercase + string.digits): | ||
return ''.join(random.SystemRandom().choice(chars) for _ in range(size)) | ||
return result + '|' + generator() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function supposed to return a valid python function name and |
is not a valid character.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the point of this change. There's no need for it to return a valid python function name. See #278 for how it's used.
Longer version: this is used to create a view name for flask to use, and flask doesn't really care whether it's a function name or any other string.
Ok, I see what is your suggestion here. |
Since that got lost (outdated diff and all that), the use case is #278.
Swagger is a bit stricter, in that each operationId must be unique, and operationIds are interpreted as function names. So the previous implementation makes perfect sense from a Swagger point of view, but not so much from a Flask point of view. So what's the use case? In order not to replicate a huge amount of code, #278 installs the same error handler for all operations with a missing operationId, which generates a 501. The PR hooks into things at a point where connexion still interprets the operationId a little, i.e. expects a unique one, but then just passes it on as a flask view name. #278 could be refactored. But when I looked into that, the code changes would have been much bigger, and re-use far less of what's already there. |
👍 |
if randomize is None: | ||
return result | ||
|
||
def generator(size=randomize, chars=string.ascii_uppercase + string.digits): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason why this is a local function (and not just a local variable holding the random string)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No good one, mostly this was too much for me for a single line:
''.join(random.SystemRandom().choice(string.ascii_uppercase + string.digits) for _ in range(randomize))
But there's a few other ways it could be broken up, sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not 100% sure but I assumed it is like that for code clarity, everything inside that is related to generating a random string. The code wouldn't be very clear without this contextualized part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 246d566 (I can revert that again, of course).
👍 |
return result | ||
|
||
chars = string.ascii_uppercase + string.digits | ||
return result + '|' + ''.join( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you changed, maybe then move this concatenation (+ '|' +
) to the ''.join(...
?
Swagger is clear on the fact that operationIds must be unique across a spec. That leads to connexion assuming that there is no reason for the same Python function to be mapped to multiple paths, and using the output of
flaskify_endpoint
for unique flask view names.Now this is correct as connexion is currently implemented. I'm preparing another PR based on #274 that would no longer make this assumption always true.
Changes proposed in this pull request: