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

Fix for Equals and notEquals Expressions. Added Support for toLower and toUpper Functions. #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jordanparrott96
Copy link

Hi,

I stumbled across your package and found it useful for oData to MongoDB conversions. My main problem with it however arose from the lack of toUpper and toLower function support.

From this pull request: #11. As the pull request has been lying dormant I added in support for these methods (and associated tests) and as our database is case sensitive, I effectively pass them straight through. Happy to discuss an alternative for these functions. I also ran into an issue where the context identifier was being deleted after a method so it couldn't be used in any other operation. The only case I saw that this was needed was for the contains method. Again happy to discuss this.

I updated the test expression numbers to the newest documentation I could find while I was at it. https://docs.oasis-open.org/odata/odata/v4.01/odata-v4.01-part2-url-conventions.html#_Toc31361028

  • Added toUpper and toLower Function support.

  • Fixed an issue where the Method Call was prematurely removing context identifier for use in other functions.

  • Updated test expression numbers to oDatav4.0.1 documentation.

  • Added tests to support the toLower, toUpper and not functions.

…Method Call was prematurely removing context identifier for use in other functions. Updated test expression numbers to oDatav4.0.1 documentation. Added tests to support the toLower, toUpper and not functions.
Copy link
Contributor

@aaghevli aaghevli left a comment

Choose a reason for hiding this comment

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

Thank you! I am not quite sure why the need for the $eq is now necessary in the tests, but this looks good to me.

I am not an original maintainer, so you may wish to see if anyone else wants to comment (maybe a few days), but I think this looks straight forwards and the added tests and updates are great.

@Uzlopak
Copy link

Uzlopak commented Nov 30, 2020

Unfortunately jaysoft is kind of dead. They don't merge anything. I will probably fork this for our company. Kind of interested on more changes if you provide them.

@aaghevli
Copy link
Contributor

aaghevli commented Nov 30, 2020

Yeah same. If there is a fork, it would be nice to keep it somewhat combined and possibly archive this. I am not using the package much anymore, but find it to be nice and useful, so can volunteer some time to reviews.

Assuming nobody objects, I can merge this this week.

@jordanparrott96
Copy link
Author

Thank you! I am not quite sure why the need for the $eq is now necessary in the tests, but this looks good to me.

The $eq was to account for problems that arose with the use of $not and other functions which effectively wrap the query. As

{displayName: {$eq: 'name'} } is equivalent to {displayName: 'name' }

When a function like 'not' was used the original program would produce the following (which is an error in mongo),

{displayName: {$not : 'name'}}

With the implemented change (this works as intentional in mongo):

{displayName: {$not: {$eq: 'name'}}}

So the program now produces {displayName: {$eq: 'name'} } instead of {displayName: 'name' }. So I had to change the tests to acknowledge this change.

@jordanparrott96
Copy link
Author

Yeah same. If there is a fork, it would be nice to keep it somewhat combined and possibly archive this. I am not using the package much anymore, but find it to be nice and useful, so can volunteer some time to reviews.

I'm more than happy to work on a forked repo of this or base a package on this, if this is indeed not being maintained anymore. I'll need to implement more features of oData into a mongoDB like conversion going forward anyway.

@aaghevli
Copy link
Contributor

Another option is to open up contributions to this repo. I am not sure what the best path forwards is, but I am open to help.

@jordanparrott96
Copy link
Author

Alright, I'll continue development on my fork in the meantime (as I still require a couple features going forward). If the decision is made to open this repo up for contributions I'd be open to help review as well.

@aaghevli
Copy link
Contributor

aaghevli commented Dec 1, 2020

Oh no. I actually do not have write access to this. I thought I did since I am not sure why I would be getting notifications otherwise. Let me know if you do fork it permanently since I am not sure jaystack will be really responsive.

@Uzlopak
Copy link

Uzlopak commented Dec 2, 2020

Does somebody of you have a solution for $count?

I think that it actually start to stream all results when doing a count :/

I have a test dataset with 5 mio documents, which are only streamed to return the number of documents.

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