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

Update to Typesense 0.25 #30

Merged
merged 18 commits into from
Feb 6, 2024
Merged

Conversation

willtempleton
Copy link
Contributor

@willtempleton willtempleton commented Jan 31, 2024

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

Copy link
Member

@jasonbosco jasonbosco left a comment

Choose a reason for hiding this comment

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

The changes look good.

Could you add a test case for the new Analytics endpoint?

I also made some additional changes in typesense:master to make use of 0.25.2 of Typesense via docker. Could you merge that branch into your fork once more?

@willtempleton
Copy link
Contributor Author

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
Copy link
Member

The JSON response is simply { name : nameValue }

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

@willtempleton
Copy link
Contributor Author

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

@jasonbosco
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

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

@willtempleton
Copy link
Contributor Author

willtempleton commented Feb 3, 2024

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
Copy link
Member

Awesome, thank you @willtempleton!

@jasonbosco jasonbosco merged commit b28f8a6 into typesense:master Feb 6, 2024
1 check passed
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.

2 participants