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.

Sign up for GitHub

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

Jump to bottom

Adding generateTenantToken method to the client #345

Merged
merged 8 commits into from
Mar 9, 2022
Merged

Adding generateTenantToken method to the client #345

merged 8 commits into from
Mar 9, 2022

Conversation

alallema
Copy link
Contributor

@alallema alallema commented Mar 2, 2022

Tenant tokens

Introduction of the new method generateTenantToken in order to facilitate the generation of the tenant token.

Related to:

brunoocasali
brunoocasali reviewed Mar 2, 2022
View reviewed changes
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

Good Job @alallema ☕

src/main/java/com/meilisearch/sdk/Client.java Outdated Show resolved Hide resolved
src/main/java/com/meilisearch/sdk/Client.java Outdated Show resolved Hide resolved
src/main/java/com/meilisearch/sdk/Client.java Outdated Show resolved Hide resolved
src/main/java/com/meilisearch/sdk/Key.java Show resolved Hide resolved
src/main/java/com/meilisearch/sdk/TenantTokenOptions.java
this.apiKey = apiKey;
}

public Date getExpiresAt() {
Copy link
Member

Choose a reason for hiding this comment

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

This Date can handle time as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

If you want to deal with Date in ruby you can use Time or Date, but if you want to deal with both you cannot use Date. My question is related to the time part of a date and if this Date class behaves like Ruby's or not 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be quite similar because the Class Date encapsulates the Date and Time but the Time is just used for (hour, minutes, second, millisecond ...) I'm not really sure you can use Time for a Date if you need a Date.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless if you check for which type of instances the object is, date or time, and it if is time you can add the time to the current timestamp. But this might be out of the minimal aspect of the helper

src/test/java/com/meilisearch/integration/TenantTokenTest.java Outdated Show resolved Hide resolved
src/test/java/com/meilisearch/integration/TenantTokenTest.java Outdated Show resolved Hide resolved
src/test/java/com/meilisearch/integration/TenantTokenTest.java

/** Test Create Tenant Token with bad expireation date */
@Test
public void testGenerateTenantTokenWithBadExpiresAt() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test about UTC as well?

Copy link
Contributor Author

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 know what you think about it?

Copy link
Member

Choose a reason for hiding this comment

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

I've added validations to expiresAt to check if they come as UTC value both in dart and ruby, I don't know if this is overkill (because the user can have problems with that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, get it. Yes, I'm not really comfortable adding this verification I'm afraid it could create errors on the user side depending on how he prefers to use this function. I think a note in the definition of the function is enough no?

@alallema @brunoocasali
Change bad naming in searchRules
3972720 
Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
@alallema @brunoocasali
Fix typo
de6d93f 
Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
brunoocasali
brunoocasali reviewed Mar 3, 2022
View reviewed changes
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

I left some points for you @alallema good job!

src/main/java/com/meilisearch/sdk/Client.java Outdated Show resolved Hide resolved
src/main/java/com/meilisearch/sdk/Client.java Outdated Show resolved Hide resolved
src/main/java/com/meilisearch/sdk/Client.java Show resolved Hide resolved
src/main/java/com/meilisearch/sdk/Key.java Show resolved Hide resolved
src/test/java/com/meilisearch/integration/TenantTokenTest.java Outdated Show resolved Hide resolved
src/test/java/com/meilisearch/integration/classes/AbstractIT.java Show resolved Hide resolved
bidoubiwa
bidoubiwa approved these changes Mar 7, 2022
View reviewed changes
Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

LGTM 🔥🔥🔥

@alallema alallema merged commit d32ab22 into bump-meilisearch-v0.26.0 Mar 9, 2022
@alallema alallema deleted the generate_token branch March 9, 2022 14:47
@alallema alallema mentioned this pull request Mar 9, 2022
Merged
@alallema alallema added the enhancement New feature or request label Mar 9, 2022
bors bot added a commit that referenced this pull request Mar 14, 2022
@bors @meili-bot @alallema
Merge #339
66f45b8 
339: Changes related to the next Meilisearch release (v0.26.0) r=alallema a=meili-bot

Related to this issue: meilisearch/integration-guides#181

This PR:
- gathers the changes related to the next Meilisearch release (v0.26.0) so that this package is ready when the official release is out.
- should pass the tests against the [latest pre-release of Meilisearch](https://github.com/meilisearch/meilisearch/releases).
- might eventually contain test failures until the Meilisearch v0.26.0 is out.

⚠️ This PR should NOT be merged until the next release of Meilisearch (v0.26.0) is out.

_This PR is auto-generated for the [pre-release week](https://github.com/meilisearch/integration-guides/blob/master/guides/pre-release-week.md) purpose._

Done:
- #341 
- #344 
- #345 
- #348

Co-authored-by: meili-bot <74670311+meili-bot@users.noreply.github.com>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
Co-authored-by: alallema <amelie@meilisearch.com>
Co-authored-by: Amélie <alallema@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bidoubiwa bidoubiwa bidoubiwa approved these changes

@brunoocasali brunoocasali Awaiting requested review from brunoocasali

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alallema @brunoocasali @bidoubiwa