search

hapijs / good

hapi process monitoring
Other
525 stars 161 forks source link

added validations for pipelines fix #525 #526

Closed yonjah closed 7 years ago

yonjah commented 8 years ago

I tried to add the check without changing the existing code much.

I had to change some of the existing tests and reporters fixtures since they were creating invalid pipelines

arb commented 8 years ago

This is more complicated than I had in mind. I was just going to do a checksum of the pipeline. Start at 1 for the initial good write steam, +0 for every transform stream and -1 for every write stream. I realize the error messaging wouldn't be as robust as what you've done here, but I really don't think it's necessary.

yonjah commented 8 years ago

I'm not sure I understand how to implement it the way you describe. What's the initial write stream ? I thought there should only be one write stream at the end of the pipeline. Also if each transform stream ads 0 for the chain how can you check it is not in the end ? What do you consider a valid checksum ? I guess I can only check the last stream to see that is a write stream but not a transform stream. That of course wont validate the entire pipeline, but that might be good enough to prevent the silent errors with the transform streams.

arb commented 8 years ago

Sorry; start at checksum at 1 for the initial READ stream from good. Plus 0 for every transform stream in the pipe and -1 for every write stream. Anything != 0 is invalid.

This approach will catch people not having a write stream or having more than 1 write stream which I think is the biggest problem since then you've got an unbounded buffer just buffering more and more messages.

yonjah commented 8 years ago

I reorganized the code a bit to make it simpler and more readble. The only difference in functionality now is that if stdin is passed to the pipeline for some reason it is still treated as a writable stream (bu I think if we wanna validate that we should probably just just white list 'stdout' and 'stderror' )

I think I understand what you mean with the checksum, but since the majority of the validation is figuring the type of stream I'm not sure if it will be much simpler.

arb commented 8 years ago

This is more than I had in mind. I wasn't going to keep track of indexes or where there was a problem, just look over them and generate a checksum and report an error for the whole pipeline based on the checksum value.

arb commented 7 years ago

Thanks for the PR, but this is way more complicated than I had imagined and it's just been sitting here idle for a while now.

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.