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

Add missing operations to MongoDB IN Node to emit status on all operations #517

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mickev36
Copy link

@mickev36 mickev36 commented Feb 4, 2019

  • every operation emits an output with the query result or an error object

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.

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

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

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the mailing list/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

@coveralls
Copy link

coveralls commented Feb 4, 2019

Coverage Status

Coverage increased (+0.04%) to 65.821% when pulling 5a62f5b on mickev36:master into 8178153 on node-red:master.

@knolleary
Copy link
Member

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.

@mickev36
Copy link
Author

mickev36 commented Feb 6, 2019

Hi,
Thanks for your answer. No problem missing my post, we're all busy 👍

I understand that introducing this breaking change is not possible.
What you describe seems great, i'm willing to implement it (add missing operations to the IN node to output stuff, while avoiding having code duplicated between the two nodes).

If it's ok on your side then I'll be working on it and update this PR

@mickev36 mickev36 changed the title Updated mongodb nodes : Merged IN and OUT nodes Add missing operations to MongoDB IN Node to emit status on all operations Feb 12, 2019
@mickev36
Copy link
Author

Hi @knolleary @dceejay

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 😄

@mickev36
Copy link
Author

Hi @knolleary @dceejay

Any update on the subject ? :-)

Cheers

@duartegarin
Copy link

Hi guys,
Also facing the exact same issue.
Any updates on this?

@albfan
Copy link

albfan commented Nov 27, 2019

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.

@mickev36
Copy link
Author

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 !

@ruralcoder
Copy link

ruralcoder commented Dec 24, 2020

Any updates on this? Nodes are useless unless you get feedback or the newly created _id.

I patched the node to my needs and it's awesome.

Screen Shot 2020-12-24 at 2 07 54 AM

@mickev36
Copy link
Author

mickev36 commented Dec 24, 2020

Any updates on this? Nodes are useless unless you get feedback or the newly created _id.

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
It seems like there's been updates on this node since, and a merge is required so i'm not 100% sure it still works, but if it doesn't, please send me a message i'll look into it.

@knolleary, should I work on this ? or maintain and recommend my forked repository ?

Merry christmas to you

@knolleary
Copy link
Member

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.

@mickev36
Copy link
Author

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.

@knolleary
Copy link
Member

@mickev36 Updating the branch would be very welcome

@mickev36
Copy link
Author

i'll look into it by the end of the week :-)

@mickev36
Copy link
Author

mickev36 commented Jan 2, 2021

Hello @knolleary ,

I've merged the newest changes in my repo, and fixed all the formatting issues so the diff is more clear.
Some CI checks fail, but it's not related to my node, something might be off on the CI side.
Please let me know if you need anything else

Have a good day

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.

6 participants