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

Change tide to actix-web #601

Merged
merged 21 commits into from
Apr 28, 2020
Merged

Change tide to actix-web #601

merged 21 commits into from
Apr 28, 2020

Conversation

qdequele
Copy link
Member

@qdequele qdequele commented Apr 9, 2020

Todo:

  • Finish changing all handlers
    • Document
    • Index
    • Health
    • Key
    • Search
    • Setting
    • Stats
    • Stop Word
    • Synonym
  • Change Params tuples to Struct (test)
  • Add middleware CORS+Compression
  • Add authorization middleware
  • Make all test work
  • Make the error handling simple

Out of scope work done:

  • Remove search multi index

Before review:

  • Check that handlers return correct status code
  • Check all unwrap()
  • Check usages of .clone()
  • Revert index.document() to the older code
  • Check all test
  • Check that no code/feature has been added. Only the changes needed for the PR

@qdequele qdequele force-pushed the tide-to-actix-web branch 8 times, most recently from 22df639 to 9d30f9e Compare April 17, 2020 14:48
@qdequele qdequele marked this pull request as ready for review April 17, 2020 14:51
@qdequele qdequele self-assigned this Apr 17, 2020
meilisearch-http/src/lib.rs Outdated Show resolved Hide resolved
meilisearch-http/src/main.rs Outdated Show resolved Hide resolved
meilisearch-http/src/main.rs Show resolved Hide resolved
meilisearch-http/src/error.rs Outdated Show resolved Hide resolved
meilisearch-http/src/error.rs Outdated Show resolved Hide resolved
meilisearch-http/src/helpers/meilisearch.rs Outdated Show resolved Hide resolved
meilisearch-http/src/helpers/meilisearch.rs Outdated Show resolved Hide resolved
@qdequele qdequele marked this pull request as draft April 21, 2020 07:26
@MarinPostma MarinPostma mentioned this pull request Apr 22, 2020
8 tasks
@qdequele qdequele marked this pull request as ready for review April 22, 2020 16:26
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

What's the difference between web::Json() and HttpResponse::Ok().json()?

meilisearch-http/Cargo.toml Outdated Show resolved Hide resolved
meilisearch-http/src/error.rs Outdated Show resolved Hide resolved
meilisearch-http/src/error.rs Outdated Show resolved Hide resolved
meilisearch-http/src/helpers/meilisearch.rs Outdated Show resolved Hide resolved
meilisearch-http/src/main.rs Outdated Show resolved Hide resolved
meilisearch-http/src/routes/synonym.rs Show resolved Hide resolved
meilisearch-http/src/routes/synonym.rs Show resolved Hide resolved
meilisearch-http/src/helpers/authentication.rs Outdated Show resolved Hide resolved
@qdequele qdequele force-pushed the tide-to-actix-web branch 2 times, most recently from 7bfaa60 to 27ae401 Compare April 24, 2020 13:09
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

BTW do we have beautiful serde error messages when the input JSON objects are wrong now?

I confirm that the code is much better and easier to read therefore easier to maintain, even the wrap thing that you made you guys (@qdequele and @MarinPostma) is awesome! Good Job!

meilisearch-http/src/routes/document.rs Outdated Show resolved Hide resolved
meilisearch-http/src/routes/index.rs Show resolved Hide resolved
meilisearch-http/src/routes/search.rs Outdated Show resolved Hide resolved
@qdequele qdequele force-pushed the tide-to-actix-web branch 2 times, most recently from 93ba8dd to 5cbadb4 Compare April 26, 2020 18:54
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Could you rebase on master, now that I have released the v0.10.1, please?

@qdequele
Copy link
Member Author

@Kerollmops done.

@Kerollmops Kerollmops merged commit 899559a into master Apr 28, 2020
@Kerollmops
Copy link
Member

So we are now on actix-web, congratulation 🎉

erlend-sh added a commit to erlend-sh/examples that referenced this pull request Apr 29, 2020
MeiliSearch very recently migrated from Tide to actix-web: meilisearch/meilisearch#601
@curquiza curquiza deleted the tide-to-actix-web branch March 30, 2021 14:34
bors bot added a commit that referenced this pull request Jan 16, 2023
601: Introduce snapshot tests r=Kerollmops a=loiclec

# Pull Request
## What does this PR do?
Introduce snapshot tests into milli, by using the `insta` crate. This implements the idea described by #597 

See: [insta.rs](https://insta.rs)

## Design
There is now a new file, `snapshot_tests.rs`, which is compiled only under `#[cfg(test)]`. It exposes the `db_snap!` macro, which is used to snapshot the content of a database.

When running `cargo test`, `insta` will check that the value of the current snapshot is the same as the previous one (on the file system). If they are the same, the test passes. If they are different, the test fails and you are asked to review the new snapshot to approve or reject it.

We don't want to save very large snapshots to the file system, because it will pollute the git repository and increase its size too much. Instead, we only save their `md5` hashes under the name `<snapshot_name>.hash.snap`. There is a new environment variable called `MILLI_TEST_FULL_SNAPS` which can be set to `true` in order to *also* save the full content of the snapshot under the name `<snapshot_name>.full.snap`. However, snapshots with the extension `.full.snap` are never saved to the git repository.

## Example
```rust
// In e.g. facets.rs
#[test]
fn my_test() {
    // create an index
    let index = TempIndex::new():
    index.add_documents(...);
    index.update_settings(|settings| ...);
    
    // then snapshot the content of one of its databases
    // the snapshot will be saved at the current folder under facets.rs/my_test/facet_id_string_docids.snap
    db_snap!(index, facet_id_string_docids);

    index.add_documents(...);   

    // we can also name the snapshot to ensure there is no conflict
    // this snapshot will be saved at facets.rs/my_test/updated/facet_id_string_docids.snap
    db_snap!(index, facet_id_string, docids, "updated");
    
    // and we can also use "inline" snapshots, which insert their content in the given string literal
    db_snap!(index, field_distributions, `@"");`
    // once the snapshot is approved, it will automatically get transformed to, e.g.:
    // db_snap!(index, field_distributions, `@"`
    // my_facet        21
    // other_field     3
    // ");
    
    // now let's add **many** documents
    index.add_documents(...);
    
    // because the snapshot is too big, its hash is saved instead
    // if the MILLI_TEST_FULL_SNAPS env variable is set to true, then the full snapshot will also be saved
    // at facets.rs/my_test/large/facet_id_string_docids.full.snap
    db_snap!(index, facet_id_string_docids, "large", `@"5348bbc46b5384455b6a900666d2a502");`
}
```

Co-authored-by: Loïc Lecrenier <loic@meilisearch.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants