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.

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

INTMDB-17: Update search #223

Merged
merged 3 commits into from Jun 23, 2021
Merged

INTMDB-17: Update search #223

merged 3 commits into from Jun 23, 2021

Conversation

abner-dou
Copy link
Contributor

@abner-dou abner-dou commented Jun 23, 2021

Description

Added properties to search index and search analyzer struct in order to complain with the current mongodb atlas API documentation

Link to any related issue(s):

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Required Checklist:

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code

Further comments

@abner-dou abner-dou requested a review from a team as a code owner June 23, 2021 07:05
@abner-dou abner-dou requested review from foesa and removed request for a team June 23, 2021 07:05
@threebee threebee changed the title Intmdb 17 update search INTMDB-17: update search Jun 23, 2021
@threebee threebee changed the title INTMDB-17: update search INTMDB-17: Update search Jun 23, 2021
Copy link

@threebee threebee left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -281,7 +283,8 @@ type IndexField struct {
// SearchAnalyzer custom analyzer definition.
type SearchAnalyzer struct {
BaseAnalyzer string `json:"baseAnalyzer"`
MaxTokenLength *float64 `json:"maxTokenLength,omitempty"`
MaxTokenLength *int `json:"maxTokenLength,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason for this change? it would be better if we don't but not a blocker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the API documentation for search kndexes and analyzers, this property is an int type.

And that makes more sense (how you can limit a token using a float value?)

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

not a blocker like I said but it's a breaking change so trying to avoid it if we can, since for json all number fields are technically float, the previous implementation was valid (even if wrong) but feel free to merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, It's good to know, so this time I'll merge that change

Thanks for your comment Gustavo

@gssbzn gssbzn added the breaking Pull requests that breaks backwards compatibility label Jun 23, 2021
Copy link
Collaborator

@themantissa themantissa left a comment

Choose a reason for hiding this comment

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

LGTM but first needs Gustavo's question answered on the reason for the float to int change.

@abner-dou abner-dou merged commit b374654 into master Jun 23, 2021
abner-dou added a commit that referenced this pull request Jun 23, 2021
@abner-dou abner-dou deleted the INTMDB-17-Update-search branch June 29, 2021 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Pull requests that breaks backwards compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants