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

Add Result container #350

Merged
merged 14 commits into from
Sep 14, 2023
Merged

Add Result container #350

merged 14 commits into from
Sep 14, 2023

Conversation

ahmednfwela
Copy link
Collaborator

@ahmednfwela ahmednfwela commented Sep 1, 2023

Pull Request

Related issue

Fixes #335
Fixes #337
Fixes #338
Starts work on #276

What does this PR do?

In this PR the main feature is adding MeiliDocumentContainer<T>, which wraps around documents returned by meilisearch and adds typed, useful information to them.

I have also added a dependency on json_serializable to generate serialization information for models (toJson + fromJson), but I will be rolling all models gradually.

Here is the complete list of changes:

  1. chore: updated CONTRIBUTING.md to include instructions on generating json_serializable files.
  2. feat: added getExperimentalFeatures and updateExperimentalFeatures to MeiliSearchClient
  3. feat: added to SearchQuery and IndexSearchQuery
    3.1. List? vector
    3.2. bool? showRankingScore
    3.3. bool? showRankingScoreDetails
  4. feat: added MeiliDocumentContainer<T>
  5. feat: added Map<String, dynamic> src to Searcheable, which exposes the raw json object returned from the server.
    REASON: just in case we don't keep up with new meilisearch releases, the user has a way to access new features.
  6. [BREAKING] fix: Searcheable<T> had a wrong matchesPosition property, which I have moved into MeiliDocumentContainer<T>
  7. [BREAKING] fix: Marked all the fields in Searcheable<T> constructor as required
    REASON: just in case we forget to add them in PaginatedSearchResult<T> or SearchResult<T>

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?
  • Updates to .code-samples.meilisearch.yml files

Thank you so much for contributing to Meilisearch!

@ahmednfwela ahmednfwela changed the title Result container Add Result container Sep 1, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2023

Codecov Report

Patch coverage: 70.06% and project coverage change: -0.81% ⚠️

Comparison is base (758a4b8) 82.84% compared to head (6ebd988) 82.03%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #350      +/-   ##
==========================================
- Coverage   82.84%   82.03%   -0.81%     
==========================================
  Files          48       57       +9     
  Lines        1218     1353     +135     
==========================================
+ Hits         1009     1110     +101     
- Misses        209      243      +34     
Files Changed Coverage Δ
lib/src/query_parameters/index_search_query.dart 19.35% <0.00%> (-2.08%) ⬇️
lib/src/results/paginated_search_result.dart 46.42% <25.00%> (-5.43%) ⬇️
lib/src/results/ranking_rules/base.dart 33.33% <33.33%> (ø)
lib/src/results/searchable.dart 73.33% <33.33%> (-26.67%) ⬇️
lib/src/results/searchable_helpers.dart 75.00% <33.33%> (ø)
lib/src/query_parameters/search_query.dart 50.00% <50.00%> (ø)
lib/src/results/document_container.dart 67.30% <67.30%> (ø)
lib/src/results/experimental_features.dart 73.33% <73.33%> (ø)
lib/src/results/experimental_features.g.dart 100.00% <100.00%> (ø)
lib/src/results/ranking_rules/attribute.dart 100.00% <100.00%> (ø)
... and 5 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator Author

@ahmednfwela ahmednfwela left a comment

Choose a reason for hiding this comment

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

Added some comments to help reviewers :)

@@ -71,9 +71,9 @@ Each PR should pass the tests and the linter to be accepted.
# Tests
curl -L https://install.meilisearch.com | sh # download Meilisearch
./meilisearch --master-key=masterKey --no-analytics # run Meilisearch
pub run test --concurrency=4
dart test
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dart test by default uses concurrency

@@ -62,6 +65,9 @@ class IndexSearchQuery extends SearchQuery {
String? highlightPostTag,
MatchingStrategy? matchingStrategy,
List<String>? attributesToSearchOn,
bool? showRankingScore,
List<dynamic /* double | List<double> */ >? vector,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wish dart had type unions 😢

final MeiliRankingScoreDetails? rankingScoreDetails;

/// Contains the location of each occurrence of queried terms across all fields
final Map<String, List<MatchPosition>>? matchesPosition;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was incorrectly placed in Searcheable<T>

Comment on lines +35 to +38
dynamic operator [](String key) => src[key];
dynamic getFormatted(String key) => formatted?[key];

dynamic getFormattedOrSrc(String key) => getFormatted(key) ?? this[key];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

convenience methods

Comment on lines +138 to +142
for (var custom in src.entries
.where((element) => !reservedKeys.contains(element.key)))
custom.key: MeiliRankingScoreDetailsCustomRule.fromJson(
custom.value as Map<String, dynamic>,
)
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 haven't had a chance to test this, since I have no idea when a custom rule is returned

this.scoreDetails,
});

Map<String, dynamic> toJson() => _$UpdateExperimentalFeaturesToJson(this);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this method is generated using json_serializable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this entire file is generated using json_serializable

@@ -40,6 +33,7 @@ class PaginatedSearchResult<T> extends Searcheable<T> {
String? indexUid,
}) {
return PaginatedSearchResult(
src: map,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we pass the original map here, if users want access to the raw response.

lib/src/results/ranking_rules/attribute.dart Show resolved Hide resolved
test/utils/books_data.dart Show resolved Hide resolved
/// - `exactMatch`: the document contains an attribute that exactly matches the query.
/// - `matchesStart`: the document contains an attribute that exactly starts with the query.
/// - `noExactMatch`: any other document.
final String matchType;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe use an enum here?

brunoocasali
brunoocasali previously approved these changes Sep 5, 2023
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.

Just a small remark about the experimental methods, otherwise I think we are good to go!

@@ -125,6 +125,35 @@ class MeiliSearchClient {
return Task.fromMap(response.data!);
}

/// Get the status of all experimental features that can be toggled at runtime
@RequiredMeiliServerVersion('1.3.0')
Future<ExperimentalFeatures> getExperimentalFeatures() async {
Copy link
Member

Choose a reason for hiding this comment

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

We did not provide these kinds of methods to the other SDKs. I understand they are very convenient during development but we believe they are not useful after that, since you only enable/disable once :)

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 can mark them as internal as a warning to users to not use this

Copy link
Member

Choose a reason for hiding this comment

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

I think that's ok to keep, my concern is because it is more code to maintain and it will be present only in this SDK :)

@brunoocasali brunoocasali added the breaking-change The related changes are breaking for the users label Sep 14, 2023
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.

🎉 🎉 🎉
bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Sep 14, 2023

Build succeeded:

@ahmednfwela ahmednfwela merged commit 656bd85 into main Sep 14, 2023
4 of 6 checks passed
@ahmednfwela ahmednfwela deleted the result-container branch September 14, 2023 13:33
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
Projects
None yet
3 participants