typesense / typesense-go

Go client for Typesense: https://github.com/typesense/typesense
Apache License 2.0
208 stars 55 forks source link

Update to latest openapi.yml spec with support for filter_by when updating documents #133

Closed sashahilton00 closed 1 year ago

sashahilton00 commented 1 year ago

Change Summary

rebuilds the client using the latest API spec currently pending PR approval #34

This adds support for the PATCH /collections/:collection_name/documents endpoint, where filter_by can be used to filter and update multiple documents.

The unit tests have also been updated to reflect changes to the spec, please review for accuracy/expected behavoiur before merge.

One thing to note, I'm not sure if it's an issue with my OpenAPI generator or some nuance of OpenAPI itself, but when generating the new client, the filter_by parameter is wrapped in a UpdateDocumentParameters struct, which is wrapped in another struct, UpdateDocumentParams - this leads to the following ugly struct initialisation when using the new endpoint to pass the filter:

&api.UpdateDocumentsParams{
    UpdateDocumentsParameters: &struct {
        FilterBy *string `json:"filter_by,omitempty"`
    }{
        FilterBy: pointer.String(filter),
    },
}

given that this is a less than pleasant UX when one wishes to use it, this ugliness is contained within an unexported function, updateDocuments and called from the exported interface, such that the exported interface is:

Update(updateFields interface{}, filter string) (int, error)

If this is an unnecessary fix due to some sort of generator error on my behalf, let me know what needs to be changed on my side and I'll clean it up.

PR Checklist

kishorenc commented 1 year ago

One thing to note, I'm not sure if it's an issue with my OpenAPI generator or some nuance of OpenAPI itself, but when generating the new client, the filter_by parameter is wrapped in a UpdateDocumentParameters struct, which is wrapped in another struct, UpdateDocumentParams

Yes, this is a bit of an issue with the openapi generator for the Go client I think. I merged your openapi spec change, but can we just have the filter_by param as a plain top level field instead of being under updateDocumentsParameters object here: https://github.com/typesense/typesense-api-spec/pull/34/files#diff-5016bdc926b6c0f649f363739d09159a1db664aae19a08fa6b38b03406f040dcR258

That would help avoid the nesting.

kishorenc commented 1 year ago

@sashahilton00

I forgot to mention that to address this shortcoming of the generator creating those wrappers, we wrote a script to unwrap the objects manually before generating the code. See here: https://github.com/typesense/typesense-go/blob/e4ab9f73e8e5f3cd32871a8d1c3bba60fa023050/typesense/api/generator/main.go#L46

We could add code to do the same here as well.

sashahilton00 commented 1 year ago

Ah, that makes sense, I didn't really look at the generator code, will refactor and squash.

sashahilton00 commented 1 year ago

All done. The commit is a bit chunky due to needing to update the tests to accomodate the recent changes to the spec in addition to adding the functionality.