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 gsub to replace invalid json values with a 1 #468

Merged
merged 5 commits into from Oct 2, 2018

Conversation

TomRitserveldt
Copy link
Contributor

@TomRitserveldt TomRitserveldt commented Jun 6, 2018

See issue: #447

@rnelson0
Copy link
Sponsor Member

rnelson0 commented Jun 6, 2018

I do not imagine it would cause a problem, but can anyone confirm that with MongoDB <3.6.3, this change will cause no issues?

@TomRitserveldt TomRitserveldt force-pushed the feature/mongodb_database_provider branch 2 times, most recently from 8f86c9e to 545abd4 Compare June 7, 2018 11:14
@TomRitserveldt
Copy link
Contributor Author

The initial change did not work for mongodb 3.2.x, I commit a new change now that moved the gsub to the mongo_eval function, since there was already something similar in place there, and in the replset function.

I merged those "dirty" fixes into the mongo_eval function so the same fix does not need to be applied everywhere seperatly. I tested this change on mongo 3.2.x and 3.6.x and it both worked fine.

should be good to merge like this @rnelson0

@TomRitserveldt TomRitserveldt force-pushed the feature/mongodb_database_provider branch from 545abd4 to 62d6e6a Compare June 8, 2018 07:19
@rnelson0
Copy link
Sponsor Member

rnelson0 commented Jun 9, 2018 via email

@TomRitserveldt
Copy link
Contributor Author

@rnelson0 sorry for late reply, yes that line is present in 2 locations, for some reason that I havent been able to find out, the tests fail when it is not present in both locations.. I've been through the code but I can't see why since it just uses the same function as far as I can tell.

So this just works.

@TomRitserveldt
Copy link
Contributor Author

Could someone re-review this PR and merge it?
The reason the tests would fail without the gsub in the second file was that the mocked mongo_eval data contained invalid json, while the whole point of this PR was to let mongo_eval output sanitized json data.
I changed the invalid mocked json to valid json now, which should solve the concern @rnelson0 had.

@bastelfreak
Copy link
Member

@rnelson0 can you take a look again please?

@vStone
Copy link
Contributor

vStone commented Sep 12, 2018

I would pull out the 'sanitation' into a separate function and then we could write some tests against that.

Totally unrelated to the pull request but:
With the current regexes I noticed we might be swallowing some values:

{
  "blalba": SomeType(123),
  "time": Timestamp(456, 1)
}

The first regex will result in:

  1. | 12
  2. | 456

(Notice the 3 being stripped from 123.

@TomRitserveldt TomRitserveldt force-pushed the feature/mongodb_database_provider branch from f17772a to 4490e62 Compare October 2, 2018 07:50
@TomRitserveldt TomRitserveldt force-pushed the feature/mongodb_database_provider branch from 4490e62 to 1cc2d77 Compare October 2, 2018 08:26
@vStone vStone requested review from vStone and removed request for vStone October 2, 2018 08:51
@@ -1,3 +1,4 @@
require File.expand_path(File.join(File.dirname(__FILE__), '..', 'util', 'mongodb_output'))
Copy link
Contributor

Choose a reason for hiding this comment

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

@vStone vStone merged commit 6f02139 into voxpupuli:master Oct 2, 2018
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.

None yet

4 participants