Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding generate_tenant_token method #412
Adding generate_tenant_token method #412
Changes from 5 commits
36e2161
1500c51
7d41151
f254aca
985db06
0ac90d7
5e214de
433966d
77294cb
7579c8f
5c81b20
09577a5
7f10221
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
You can handle empty strings?
''
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.
api_key = self.config.api_key if api_key is None else api_key
api_key
is a string so no need to convert.Also, the master key can't be used right?
self.config.api_key
could be the master key. There is a default search key available that can be used as a default instead.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.
@sanders41, yes I didn't convert it at first but
mypy
wasn't agree at all with it and generate this error:How can I fix this in a better way than cast it in
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.
I think mypy is complaining because
self.config.api_key
could also beNone
and there isn’t a check for that.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 can handle empty string
search_rule
isUnion[Dict[str, Any], List[str]]
not astring
. I can check if it's not an emptyDict
or an emptyList
but I'm not sure it's relevant.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.
Did you have a setup handler without documents or a simple index creation? Because it's good to avoid unnecessarily setup :D
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 had documents to check the search return at least a few documents. I find that doing a search without results was less convincing
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.
And same as above I just let this specific verification on the first one with search_rules
['*']
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.
Actually, you don't really care about the size of the response, because it's not related to the key generation itself. But you do care if the
/search
request answer with a success status!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'm not sure to understand what you mean. There is no
status
field in the search response so I just check if the response has enough results to be valid. It could for example don't have the right on myindex
or hadfilters
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.
What I want to mean is, if your request returned just 10 records caused by a new version of the engine v0.27 for example, that's an error? this test should break because of that change? In this case no, because your test just wanted to check if your generated token can make a request successfully.
But, since the assertion is
check if the returned records are equal to 20
now you have to fix this test too because the core engine changed internally.Giving more context: this idea does not apply everywhere, because there are some use-cases we will need to handle that, ie. the tests regarding the filters or the search itself.
A different approach is to use more unit testing for some cases instead of e2e tests give a look here:
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 get it you mean if the search
limit
by default is modified from 20 to 10.I can change the request with a specific limit like:
This way I'm sure the search works well and the test will not be breaking. What do you think?
Also, I just let it in the first test when the search_rule is set to
['*']