-
Notifications
You must be signed in to change notification settings - Fork 593
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
Add missing operations to MongoDB IN Node to emit status on all operations #517
base: master
Are you sure you want to change the base?
Conversation
…ts an output with the query result or an error object Discussed in https://discourse.nodered.org/t/mongodb-node-improvements/4442
Hi, we need to have more discussion about your proposal. I see neither @dceejay or myself responded to the discussion on the forum last October and I don't recall having any in depth conversation on slack about it - apologies if I'm misremembering. If we had we would have pointed out that merging existing nodes would be a massively breaking change that we wouldn't consider as the starting point for any change. A much more modest change would be to add the insert/save/update/remove options to the existing MongoDB In node and leave the Out node largely untouched (albeit with a bit of internal refactoring to reduce code duplication). That would satisfy the requirement of being able to do a write to the database and get a result back - whilst maintaining 100% backward compatibility. Apologies for missing the original forum discussion - its a worthwhile requirement to satisfy, we just need to find the right approach. |
Hi, I understand that introducing this breaking change is not possible. If it's ok on your side then I'll be working on it and update this PR |
I've updated the PR with what we discussed : I added the 4 missing operations (insert/save/update/delete) to the IN node, and emitted the mongodb operation result. I did a simple factorization so that no code is duplicated. Let me know if there's anything more 😄 |
Any update on the subject ? :-) Cheers |
Hi guys, |
Let me know if I can collaborate onn this PR. get notified when a mongo operation ends is useful for flows where mongodb input is needed. |
Hi @knolleary Is there any updates on this MR ? I see it's been removed from "To Do", is there anything that needs to be done ? Thanks in advance ! |
Hello ruralcoder, Unfortunately, I haven't heard back from the core devs in over a year and a half (1.0 related ?), so i'm guessing this will never be merged. I've forked the repository and made the change in here : https://flows.nodered.org/node/node-red-contrib-mongodb @knolleary, should I work on this ? or maintain and recommend my forked repository ? Merry christmas to you |
Hi all, My apologies for forgetting about this pr. We have lots of nodes in this repository that were written in the early days of the project, before the community could write and publish their own nodes. That does mean there are nodes in here for things we don't regularly use ourselves. This does make it tricky to test/verify changes like this as I first have to setup a mongo environment to work with. So we have a choice to make - whether we should work to make his node the "best" mongo node available, or let the community contributed nodes take that spot. For some technologies I'm more than happy for the community to take the lead. But if there's a quick win for making our mongo node more useful then, in this instance, I'm happy to spend the time on it. I'll see where I can get to over the Christmas holidays. |
Hi, Thanks for your answer. No apologies needed, I understand how work goes ;-) I understand the wish to keep one official repo for this. If there's anything I can do to help, please let me know ! I can update the branch to remove the merge conflict if that helps. Again, thanks for this awesome tool. |
@mickev36 Updating the branch would be very welcome |
i'll look into it by the end of the week :-) |
Hello @knolleary , I've merged the newest changes in my repo, and fixed all the formatting issues so the diff is more clear. Have a good day |
Discussed in https://discourse.nodered.org/t/mongodb-node-improvements/4442 and on slack with @knolleary a couple weeks ago, which proposed to merge my fork.
I had my js formatter enabled, so you might have some troube with the diff. If you cannot accept it, please let me know, I will re-do it without the code formatter.
Proposed changes
I think the nodes for node-red-node-mongodb (which is provided with the extra nodes package) should provide always provide an output, so we can process the status of the query (Did it fail ? Did it succeed ? When ?)
Done in this PR :
Merges the two mongodb nodes. Every action outputs a status (query status or error).
Checklist
grunt
to verify the unit tests pass