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

relates to #178: adapt countDocuments per spec, improve tests #202

Merged
merged 2 commits into from
Mar 1, 2023

Conversation

ivansenic
Copy link
Contributor

@ivansenic ivansenic commented Feb 28, 2023

What this PR does:

Adaptation to spec for the countDocuments, including test improvements.

  • adapted status to count as per spec, from counted_documents (@kathirsvn fyi)
  • removed option class from command as per spec no options exists
  • improved tests:
    • added resolver test
    • added error case in operation tests
    • several fixes for the integration tests:
      • assert no errors & data
      • few tests were missing @Test annotation
      • improved readability
      • remove test order - not needed
  • fixed swagger examples

Which issue(s) this PR fixes:
Relates to #178.

@maheshrajamani
Copy link
Contributor

@vkarpov15 Can you confirm if countDocuments response element has to be count or counted_documents? Remember seeing counted_documents response name in some documentation.

@vkarpov15
Copy link
Collaborator

vkarpov15 commented Feb 28, 2023

@maheshrajamani either count or counted_documents is fine as far as Mongoose is concerned, so long as await countDocuments() returns a number in the stargate-mongoose driver layer.

@tatu-at-datastax
Copy link
Contributor

tatu-at-datastax commented Feb 28, 2023

I was trying to figure out Mongo equivalent, but countDocuments() documentation mostly referes to mongosh command; like:

https://www.geeksforgeeks.org/mongodb-countdocuments-method/

but regardless seems to just return raw numeric count, not a BSON Object.
That is not a good response for various reasons, but I was hoping to find better definition of what Mongoose would expect if not plain number.

@vkarpov15 You probably know this are best, WDYT?

@ivansenic
Copy link
Contributor Author

@tatu-at-datastax @maheshrajamani I am not sure if I can merge this now? Will you provide your reviews?

@vkarpov15
Copy link
Collaborator

@tatu-at-datastax Mongoose expects countDocuments() to return a number. However, if it makes more sense for Stargate to return { count: 2 } as opposed to just 2, that's fine, I think it isn't a big deal for the stargate-mongoose driver to transform this result.

@ivansenic
Copy link
Contributor Author

OK I am merging this when I fix remaining tasks.

@tatu-at-datastax
Copy link
Contributor

tatu-at-datastax commented Mar 1, 2023

Sorry, I am fine with the change, just wanted to verify I understood if it's different from Mongo handling; and if so if it is intentional and not accidental. Since it's intentional that's fine.

And I think the benefit for us has more to do with unified response (Page object). Mongo probably has/had backwards compatibility concern to worry about, probably chose return type before more unified handling.

@ivansenic ivansenic force-pushed the ise-178-count-documents branch from 098c16e to 29d8098 Compare March 1, 2023 17:46
@ivansenic
Copy link
Contributor Author

Will merge this, and add the error test case when updating SG to v2.0.9.

@ivansenic ivansenic merged commit e6c9f3f into main Mar 1, 2023
@ivansenic ivansenic deleted the ise-178-count-documents branch March 1, 2023 17:59
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.

4 participants