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

Changes for v1.2.0 #318

Merged
merged 5 commits into from
Jun 1, 2023
Merged

Conversation

ahmednfwela
Copy link
Collaborator

@ahmednfwela ahmednfwela commented May 30, 2023

Pull Request

Related issue

Implements meilisearch/integration-guides#261

What does this PR do?

  • Introduce a new annotation RequiredMeiliServerVersion which does nothing for now, but we can use it to document at which version members are introduced.
  • Introduce filter expressions for IS NULL,IS NOT NULL, IS EMPTY, IS NOT EMPTY
  • Some internal Queryable refactoring to unify its behavior and avoid duplicating the code.
  • Added filter parameters to DocumentsQuery
  • [Breaking] Changed deleteDocuments Signature
- Future<Task> deleteDocuments(List<Object> ids)
+ Future<Task> deleteDocuments(DeleteDocumentsQuery query)

Migration guide

- index.deleteDocuments([456, 4])
+ index.deleteDocuments(DeleteDocumentsQuery(ids: [456, 4])) 

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

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.

Thanks a lot for taking care of this release @ahmednfwela 🎉

Besides the comments I've made here, I would like to highlight a particular desire that is creating smaller PRs, especially in the pre-release weeks.

We usually have to deal with a huge amount of context-switch moments, and it is incredibly painful to do a proper review when the PR goes all over the place.

These kinds of improvements will always be welcomed, but it would surely make my life much easier if you could break it into smaller PRs next time. 😅

lib/meilisearch.dart Show resolved Hide resolved
lib/src/annotations.dart Outdated Show resolved Hide resolved
lib/src/facade.dart Outdated Show resolved Hide resolved
}

@MeiliServerVersion('1.2.0')
class MeiliIsEmptyOperatorExpression extends MeiliOperatorExpressionBase {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to call Meili all the time? I don't know if it is worthy repeating that since its inside of the meilisearch dart SDK + it is not exported (as far as I remember).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's exported in the _exports file in the same folder

lib/src/index.dart Outdated Show resolved Hide resolved
lib/src/query_parameters/index_search_query.dart Outdated Show resolved Hide resolved
lib/src/query_parameters/queryable.dart Show resolved Hide resolved
test/filter_builder_test.dart Show resolved Hide resolved
test/filter_builder_test.dart Show resolved Hide resolved
test/search_test.dart Outdated Show resolved Hide resolved
@brunoocasali brunoocasali added the enhancement New feature or request label May 31, 2023
@ahmednfwela
Copy link
Collaborator Author

but it would surely make my life much easier if you could break it into smaller PRs next time.

funny enough, I actually started this PR to only add the delete/fetch methods (as can be seen from the branch name), but I forgot to switch to a different branch midway, and everything got jumbled up together, so definitely sorry about that 😅

Comment on lines 418 to 444
Future<Task> deleteDocuments([DeleteDocumentsQuery? query]) async {
Object? deleteBatchData;
Object? deleteData;
if (query != null) {
if (query.ids != null) {
// if ids are available, fallback to the delete-batch route
deleteBatchData = query.ids;
} else {
// if ids are NOT available, use the new delete route
deleteData = query.toSparseMap();
}
}

Future<Response<Map<String, Object?>>> future;
if (deleteData != null) {
future = http.postMethod(
'/indexes/$uid/documents/delete',
data: deleteData,
);
} else {
future = http.postMethod(
'/indexes/$uid/documents/delete-batch',
data: ids,
),
);
data: deleteBatchData,
);
}

return await _getTask(future);
Copy link
Member

@brunoocasali brunoocasali May 31, 2023

Choose a reason for hiding this comment

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

What do you think?

Future<Task> deleteDocuments([DeleteDocumentsQuery? query]) async {
  String route = 'delete';
  Object? data;

  if (query != null) {
    if (query.ids != null) {
      // if ids are available, fallback to the delete-batch route
      data = query.ids;
      route = '$route-batch';
    } else {
      // if ids are NOT available, use the new delete route
      data = query.toSparseMap();
    }
  }

  return await _getTask(
    http.postMethod('/indexes/$uid/documents/$route', data: data)
  );
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the logic, and also added client-side assertions to prevent bad inputs, since the server will throw on them anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

relevant commit: 47eed42

lib/src/index.dart Outdated Show resolved Hide resolved
@ahmednfwela ahmednfwela self-assigned this Jun 1, 2023
this.filter,
this.filterExpression,
}) : assert(
(ids != null && ids.isNotEmpty) ^
Copy link
Member

Choose a reason for hiding this comment

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

wow, that's cool!!

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.

LGTM!!!

Thank you, @ahmednfwela, for all your work in this repository, it is incredibly amazing to see how much this repository improved in the past months!

@brunoocasali brunoocasali merged commit c72a80f into bump-meilisearch-v1.2.0 Jun 1, 2023
@brunoocasali brunoocasali deleted the get-delete-by-filter branch June 1, 2023 12:46
@meili-bot
Copy link
Contributor

This message is sent automatically

Thank you very much for submitting your PR! Did you know that throughout the month of June we’re holding a rafle?
If you share the link to your merged PR in our #giveaway Discord channel, you’ll automatically join a lottery for a chance at winning some Meiliswag. 🙂
Don’t hesitate to join us: https://discord.com/channels/1006923006964154428/1111273670657200198

@brunoocasali brunoocasali added the breaking-change The related changes are breaking for the users label Jun 1, 2023
meili-bors bot added a commit that referenced this pull request Jun 6, 2023
323: Update version for the next release (v0.14.0) r=brunoocasali a=meili-bot

This version introduces features released on Meilisearch v1.2.0 🎉
Check out the changelog of [Meilisearch v1.2.0](https://github.com/meilisearch/meilisearch/releases/tag/v1.2.0) for more information on the changes. 
⚠️ If you want to adopt new features of this release, **update the Meilisearch server** to the according version.

Check the complete CHANGELOG here: https://github.com/meilisearch/meilisearch-dart/blob/main/CHANGELOG.md

### 💥 Breaking changes

- The method `deleteDocuments()` now supports a different behavior. This method now takes a `DeleteDocumentsQuery`, which could contain the filter or the `ids` (aka old behavior).  #318 `@ahmednfwela`  

  ⚠️ You must configure the attributes you want to filter using the `MeiliSearchIndex.filterableAttributes()`.
  ⚠️ Remember to **update your Meilisearch server to v1.2.0 or newer before** adopting it.
  
  Still, even being supported, the ability to receive only a list of ids is *deprecated*, and it will be removed soon.

  ```diff
  // from:
  - Future<Task> deleteDocuments(List<Object> ids)
  + Future<Task> deleteDocuments(DeleteDocumentsQuery query)
  
  // to:
  - index.deleteDocuments([456, 4])
  + index.deleteDocuments(DeleteDocumentsQuery(ids: [456, 4]))
  ```

- Add the ability to set `filter` in the `DocumentsQuery`. When a query with a `filter` is sent to `getDocuments(params: DocumentsQuery)` it will filter the documents like the `search` method. See [the docs on how to use filters](https://www.meilisearch.com/docs/learn/advanced/filtering#filter-basics). #318 `@ahmednfwela`  
  
  ⚠️ You must configure the attributes you want to filter using the `MeiliSearchIndex.filterableAttributes()`.
  ⚠️ Remember to **update your Meilisearch server to v1.2.0 or newer before** adopting it.


- `MeiliSearchIndex.search` now takes a `String query` and a `SearchQuery` object as the only inputs. #310 `@ahmednfwela`  
  - Replace any occurrence of search `index.search('xyz', ....)` with `index.search('xyz', SearchQuery(....))`

  ```diff
  - await client.index('books').search('query', sort: [], filter: ...);
  + await client.index('books').search('query', SearchQuery(sort: [], filter: ...));
  ```

- `Meili.geoBoundingBox` and `Meili.geoRadius` now take record values to represent the `lat`/`lng` points: #310 `@ahmednfwela`  
  ```diff
  // Confusing, unclear
  - Meili.geoBoundingBox(3,5.3,10,20)
  // Not Confusing :)
  + Meili.geoBoundingBox((lat: 3, lng: 5.3), (lat: 10, lng: 20))
  ```
  ```diff
  // Confusing, unclear
  - Meili.geoRadius(3, 5.3, 100)
  // Not Confusing :)
  + Meili.geoRadius((lat: 3, lng: 5.3), 100)
  ```
  
- Change `MultiSearchQuery.queries` to be a `List<IndexSearchQuery>` instead of a `List<SearchQuery>`: #310 `@ahmednfwela`  
  ```diff
  final result = await client.multiSearch(MultiSearchQuery(queries: [
  -      SearchQuery(
  +      IndexSearchQuery(
            query: "",
            indexUid: index1.uid,
          ),
  -    SearchQuery(
  +    IndexSearchQuery(
            query: "",
            indexUid: index2.uid,
          ),
  ];
  ```

### Enhancements:

- Introduce a new annotation `RequiredMeiliServerVersion` which documents the version these members were introduced. #310 `@ahmednfwela`  
- Introduce filter expressions for `IS NULL`, `IS NOT NULL`, `IS EMPTY`, `IS NOT EMPTY`. #310 `@ahmednfwela`  
- Added `filter`, `filterExpression` parameter to `DocumentsQuery`. #310 `@ahmednfwela`  
- Some internal `Queryable` refactoring to unify its behavior and avoid duplicating the code. #310 `@ahmednfwela` 
- Added a workaround for meilisearch/meilisearch#3740 by `jsonEncoding` attribute names when using `filterExpression`s #310 `@ahmednfwela`  
- A new type is introduced `IndexSearchQuery` which extends `SearchQuery`. #310 `@ahmednfwela`  
  - Added `copyWith` to `SearchQuery` and `IndexSearchQuery` #310 `@ahmednfwela`  

Thanks again to `@brunoocasali,` `@ahmednfwela!` 🎉


Co-authored-by: meili-bot <74670311+meili-bot@users.noreply.github.com>
Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants