pelias / pelias

Pelias is a modular open-source geocoder using Elasticsearch.
https://pelias.io
MIT License
3.22k stars 222 forks source link

Replace usage of event-stream #801

Open orangejulius opened 5 years ago

orangejulius commented 5 years ago

Pelias has long used the event-stream library, which has fallen out of date and was involved in a credentials stealing attack.

For Pelias, we mostly used it to convert small arrays into streams for testing purposes. To solve that need, something minimal like the stream-array package probably makes more sense.

event-stream is currently used in the following repositories, and we should see how much work it is to remove it:

This would be a good project for someone newer to Node.js or Pelias. We'll gladly help someone get started with pull requests.

benetis commented 5 years ago

Hi, i would like to try working on this issue

missinglink commented 5 years ago

Great @benetis that would be super helpful! Please select one of the repositories above to start on and open a Pull Request.

Once the first one is merged the other ones should be easier :)

missinglink commented 5 years ago

An easier one to start with might be https://github.com/pelias/openstreetmap

I can see that the event-stream module is only being used in one place:

➜  openstreetmap git:(master) git grep event-stream
package.json:    "event-stream": "^4.0.0",
test/stream/address_extractor.js:var event_stream = require( 'event-stream' );

Once the tests pass (by running the following commands) then please open up a PR for review:

npm test
npm run end-to-end

When you open the PR, please add a link to this issue in the description so we can track the progress.

benetis commented 5 years ago

I started looking into it, it seems that stream-array doesn't have any wrappers for writable stream. Use cases in tests seem to rely on this. Would you like me to write a small wrapper for such streams like event-stream had (writeArray) or check another library which supports this or I am looking at the problem incorrectly? Not experienced with nodejs :S

missinglink commented 5 years ago

hmm.. yes you're correct. I spent some time looking for an appropriate module that's still maintained (I'd like to avoid adding two modules to replace one).

the following worked for me:

var mock = require('stream-mock');

function test_stream(input, testedStream, callback) {
  let reader = new mock.ObjectReadableMock(input);
  let writer = new mock.ObjectWritableMock();
  writer.on('error', (e) => callback(e));
  writer.on('finish', () => callback(null, writer.data));
  reader.pipe(testedStream).pipe(writer);
}
sharkbait490 commented 4 years ago

Hi @orangejulius, @missinglink and @benetis , I would like to start on implementing this on the whosonfirst importer - if no one else is not working on this.

orangejulius commented 4 years ago

@sharkbait490 go for it :) https://github.com/pelias/openaddresses/pull/457 was recently merged which should serve as good template for the sort of changes that are involved