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

NDJSON/CSV methods to add and update documents #243

Open
4 of 13 tasks
curquiza opened this issue Oct 19, 2021 · 9 comments
Open
4 of 13 tasks

NDJSON/CSV methods to add and update documents #243

curquiza opened this issue Oct 19, 2021 · 9 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@curquiza
Copy link
Member

curquiza commented Oct 19, 2021 •
edited

⚠️ This issue is generated, it means the nameing might be done differently in this package (ex: add_documents_json instead of addDocumentsJson). Keep the already existing way of naming in this package to stay idiomatic with the language and this repository.

📣 We strongly recommend doing multiple PRs to solve all the points of this issue

MeiliSearch v0.23.0 introduces two changes:

  • new valid formats to push data files, additionally to the JSON format: CSV and NDJSON formats.
  • it enforces the Content-type header for every route requiring a payload (POST and PUT routes)

Here are the expected changes to completely close the issue:

  • Currently, the SDKs always send Content-Type: application/json to every request. Only the POST and PUT requests should send the Content-Type: application/json and not the DELETE and GET ones. Add methods to add JSON, CSV and NDJSON document  #257

  • Add the following methods and 🔥 the associated tests 🔥 to ADD the documents. Depending on the format type (csv or ndjson) the SDK should send Content-Type: application/x-dnjson or Content-Type: text/csv)

  • Add the following methods and 🔥 the associated tests 🔥 to UPDATE the documents. Depending on the format type (csv or ndjson) the SDK should send Content-Type: application/x-dnjson or Content-Type: text/csv)

    • updateDocumentsJson(string docs, string primaryKey)
    • updateDocumentsCsv(string docs, string primaryKey)
    • updateDocumentsCsvInBatches(string docs, int batchSize, string primaryKey)
    • updateDocumentsNdjson(string docs, string primaryKey)
    • updateDocumentsNdjsonInBatches(string docs, int batchSize, string primaryKey)

docs are the documents sent as String
primaryKey is the primary key of the index
batchSize is the size of the batch. Example: you can send 2000 documents in raw String in docs and ask for a batchSize of 1000, so your documents will be sent to MeiliSearch in two batches.

Example of PRs:

Related to: meilisearch/integration-guides#146

If this issue is partially/completely implemented, feel free to let us know.
@curquiza curquiza added hacktoberfest good first issue Good for newcomers enhancement New feature or request labels Oct 19, 2021
@thicolares thicolares mentioned this issue Oct 22, 2021
Merged
bors bot added a commit that referenced this issue Oct 24, 2021
@bors @thicolares
Merge #253
836d83b 
253: Remove Content-Type from non-body HTTP methods r=curquiza a=thicolares

- The non-body HTTP methods: GET and DELETE.
- Expose the headers as arg to make it explicit and move the decision/concern to each method instead of send_request.
- Use keywords args as there're multiple optional ones.
- Extract the non-body headers to a classes property to convey its intention with its name and reduce duplication.
- Duly update related test.

Closes the topic 1/3 of #243. It's a small step towards the other topics. Maybe they could even be bundled into a single PR, but I don't think it's mandatory.
>  Currently, the SDKs always send Content-Type: application/json to every request. Only the POST and PUT requests should send the Content-Type: application/json and not the DELETE and GET ones.

Co-authored-by: Thiago Colares <thicolares@gmail.com>
@thicolares
Copy link
Contributor

@curquiza qq about the new methods to ADD documents by type as I was checking the API docs:

  1. Should we keep the exiting generic add/update documents methods? I think so. For backward compatibility and also based on the other PRs listed above;
  2. I was planning to add the new methods into the MeiliSearch::Index. But by doing this, the class will become even longer and rubocop will complain too. I can bump the rubocop's limit or I can split the file. But I've noticed that file used to be split and it was merged back. Question: So, should I keep everything in MeiliSearch::Index?

@curquiza
Copy link
Member Author

curquiza commented Oct 26, 2021 •
edited

Hello @thicolares

  1. Yes, we should keep it :)
  2. I think it's ok to have long classes, not a big issue. However, maybe we can consider creating a document.rb file. Same for settings.rb. But it should not bring any complexity for the only reason of splitting the code. I would rather have one big class easy to maintain and to contribute to rather than a complex code. But I would love to review a PR suggesting improvements 🙂

@curquiza
Copy link
Member Author

👆 Related to my previous message
However, the code split should NOT be done in the PR adding the methods described in this issue. This must be done in a separated PR dedicated to the code restructuration.

@thicolares
Copy link
Contributor

thicolares commented Oct 26, 2021 •
edited

@curquiza yup, sure thing! Thanks for the answers! By default, I don't like long classes for multiple reasons, but there's no silver bullet and it may vary with the context and need. And yes, I'll try to change the minimum and stick to the scope :)

@thicolares
Copy link
Contributor

@curquiza, there's a small thing we must fix and a solution I would like to align.

To fix

This transformation to JSON introduces a small challenge to this task. And a lot of the SDK currently relies on it.

Given we may send Strings containing a JSON,NDJSON or CSV; http_request.rb won't know when to transform or not to JSON. And sometimes it won't make sense and will break. We could check the header along with the content, but again, it can be confusing and this shouldn't be the concern of the line stated above.

My suggested solution for now

Expose an option to explicitly tell when the body should be transformed or not.

Motivations

  • That's a short and clean path, as also stick with the current structure.
    • Ideally, I think the body transformation should be a concern of who knows about the content of the body, not of a generic piece of code. But I don't want to make this refactoring now.
  • Reuse the options Hash

Please, lemme know if you believe I'm missing something. My first commit will contain this change only.

@thicolares
Copy link
Contributor

@curquiza based on, does we have to ready to translate any data key in JSON, NDJSON and CSV files from snake_case to camelCase as well?

@curquiza
Copy link
Member Author

Expose an option to explicitly tell when the body should be transformed or not.

I'm not sure to concretely see what you mean, a PR would be better, but I can understand if you don't have the time :) do you mean a parameter?

@curquiza based on, does we have to ready to translate any data key in JSON, NDJSON and CSV files from snake_case to camelCase as well?

Nope, we don't touch the user input regarding their dataset :)
This PR was for the search parameters and the settings.

@thicolares
Copy link
Contributor

I'm not sure to concretely see what you mean, a PR would be better

Definitely haha! I was coding in the meantime and was just trying to anticipate. I'll push it after tweaking RuboCop.

Nope, we don't touch the user input regarding their dataset :)

Perfect. That was my understanding from the code, but the previous tickets/conversation were a bit unclear to me

@thicolares thicolares mentioned this issue Oct 27, 2021
Merged
3 tasks
meili-bors bot added a commit that referenced this issue Dec 7, 2021
@meili-bors @thicolares @curquiza
Merge #257
081b8ef 
257: Add methods to add JSON, CSV and NDJSON document  r=curquiza a=thicolares

### Addresses the following items of #243:

- [x] addDocumentsJson(string docs, string primaryKey)
- [x] addDocumentsCsv(string docs, string primaryKey)
- [x] addDocumentsNdjson(string docs, string primaryKey)

### Changes

_Tip: in order to ease the code review, you can start reviewing it commit by commit._

- Expose request options as a parameter of `http_post`. Motivations:
    - Allow who knows about the content to use `transform_body?` in order to decide if `http_request` should or not transform the body to JSON;
    - Group `transform_body?`, `headers` and [other options' default values that were spread around](https://github.com/meilisearch/meilisearch-ruby/blob/1ddbc9253f15cc4879bd6e7392c145ae9af9bde9/lib/meilisearch/http_request.rb#L82-L83) into a single Hash;
    - It can be improved in the next iterations, but it will already ease the next tasks within #243.
- Create methods to add documents as JSON, NDJSON and CSV
    - Add related tests exposing the usage of primaryKeys
    - No _"add in batches"_ included in this PR.

Co-authored-by: Thiago Colares <thicolares@gmail.com>
Co-authored-by: Clémentine Urquizar - curqui <clementine@meilisearch.com>
@brunoocasali
Copy link
Member

Since this was merged, I will close it!

@brunoocasali brunoocasali reopened this Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@thicolares @brunoocasali @curquiza @meili-bot