typesense / typesense-swift

Swift Client for Typesense ⚡️🔎
https://typesense.org/docs/
Apache License 2.0
39 stars 16 forks source link

Update to Typesense 0.25 #30

Closed willtempleton closed 9 months ago

willtempleton commented 9 months ago

Change Summary

Regenerated Typesense Models to updated API spec. Updated Search Parameters for new models. Fixed any type errors resulting from regenerated models.

PR Checklist

willtempleton commented 9 months ago

I've implemented Analytics and added the associated tests. 3/4 analytics test pass on my device with only delete failing due to a decoding error. The JSON response is simply { name : nameValue } while the decoder expects values for each element in the AnalyticsRuleSchema. If this is a valid response, I can simply change the AnalyticsRuleSchema to have optional values -- or implement a different workaround if this is not desired.

jasonbosco commented 9 months ago

The JSON response is simply { name : nameValue }

Oh yeah that's the right response. We need to update the Typesense API spec accordingly.

willtempleton commented 9 months ago

Gotcha. Made that change. Still failing all multi search and synonym tests due to "collection already exists" errors.

jasonbosco commented 9 months ago

May I know where you're seeing that error? In GitHub Actions I see a series of compilation errors, even before the test suite runs: https://github.com/typesense/typesense-swift/actions/runs/7748279634/job/21130683064#step:6:32

willtempleton commented 9 months ago

I see the test failures when I run on my Mac while emulating Typesense via docker. The compiling errors I believe are due to an older version of swift. That should be fixed with the latest commit.

jasonbosco commented 9 months ago

Yup, I see those HTTP 409 errors now in GitHub action as well...

I wonder how the same test suite works on master 🤔

May be we should add logic to the collection creation code snippets in the tests, to not error out if the collection already exists?

willtempleton commented 9 months ago

Just did that on multi search tests and all tests passed locally (didn't need to do it for synonyms oddly enough).

Unfortunately I had to remove highlight from SearchResultHit at least for the time being. Since Any does not conform to decodable, I tried to represent it as data first and then use JSONSerialization, but unfortunately the decoder recognizes the highlight field as a dictionary, throwing an error (and the compiler throws an error if highlight is declared a dictionary). I'll take a few more stabs at it if any lightbulbs click.

getNextNode() is now failing -- and has been since I implemented AnalyticsTests. Oddly enough, it seems related to analytics tests occurring first. When I run the APICallTests alone they pass. Doesn't make much sense as everything is a value type.

jasonbosco commented 9 months ago

getNextNode() is now failing -- and has been since I implemented AnalyticsTests

Looks like the last run of the test suite passed on CI?

willtempleton commented 9 months ago

My bad, last comment was a bit confusing. Changing the alphabetical order of the test classes fixed the issue with getNextNode failing. I'm not sure why the APICall tests need to go first (maybe you have better insight into that?), but all the tests now succeed.

Also added an additional test for document group search since the new found parameter was added to each group. I think everything should be good to go now.

jasonbosco commented 9 months ago

Awesome, thank you @willtempleton!