seznam / elasticlient

C++ Elasticsearch client library
https://seznam.github.io/elasticlient/
MIT License
132 stars 67 forks source link

Added option to turn off document validation #22

Closed ladislavmacoun closed 3 years ago

ladislavmacoun commented 3 years ago

For long string the validation is expensive, and often not necessary since the doc could be validated during its constructions/deserialization.

The option default to true, so previous API should not be broken.

Signed-off-by: Ladislav Macoun ladislavmacoun@gmail.com

johniez commented 3 years ago

Thanks for the PR. Nice and straightforward, the only thing to consider before merging is the ABI compatibility. For those who use it as a shared library, we probably need to split these functions with bool validate = true into separate declarations. I'm not sure if this is still necessary, but KDE bin compatibility mentions this as don't: extending a function with another parameter, even if this parameter has a default argument :-(

ladislavmacoun commented 3 years ago

Thanks for the PR. Nice and straightforward, the only thing to consider before merging is the ABI compatibility. For those who use it as a shared library, we probably need to split these functions with bool validate = true into separate declarations. I'm not sure if this is still necessary, but KDE bin compatibility mentions this as don't: extending a function with another parameter, even if this parameter has a default argument :-(

Thanks for the reply, another option that comes to my mind is to introduce new API functions like {index,update,create}DocumentNoValidation, or something like that.

Those functions would be similar to *Document, only without the validation. In this case, the ABI should not change in this case.

johniez commented 3 years ago

I believe that adding overloaded functions with extra argument bool validate, without the default value, should be ok. It can be merged together (with default value) when we will break binary compatibility. So the API would be ugly just temporarily :-)