Skip to content
This repository has been archived by the owner on Feb 28, 2022. It is now read-only.

Write protection in the pipeline not fully functional #228

Closed
koraa opened this issue Mar 27, 2019 · 5 comments
Closed

Write protection in the pipeline not fully functional #228

koraa opened this issue Mar 27, 2019 · 5 comments
Labels
bug Something isn't working released

Comments

@koraa
Copy link
Contributor

koraa commented Mar 27, 2019

The Pipeline class implements write protection, so pipeline steps cannot modify their arguments.

In order to do this, the lodash.merge function is applied to the current data before each pipeline step. This way pipeline steps/.every() steps can modify the current data, but it won't have an actual effect since they are modifying a copy.

This approach has some drawbacks:

  1. The amount of work to be done grows roughtly (pipeline_steps * datasize), which can be quite a lot
  2. This approach only work on arrays and plain objects; other types (like the DOM) can still be modified
  3. The approach has the potential to hide a lot of bugs; e.g. accidental modification which is hidden, or third party steps could be modifying the data intentionally and not notice their notifications are discarded (e.g. due to improper testing)

I can see some ways to remedy this:

  1. Allow data modification
  2. In order to remedy point (2) we could introduce a deepClone function of our own based on traits; this function would throw for unknown types instead of just not clone them. Devs could react to those exceptions by simply adding an implementation for the type
  3. We could use an immutable js lib to actually write protect the data (and throw if modification is attempted). This would necessitate a freeze/thaw API, probably trait based so it can be extended with custom types when the need arises.

Even though I would enjoy adding some more traits, I think the best way to go is to use (1); we are (unfortunately) in a language that does not natively support const attributes, so emulating them somehow is admittetly very hard. Solution 2 and 3 would both put a lot of extra work on third party devs if they want to use pipelines with their custom types.

Allowing modification is the usual thing to do in js…
@trieloff
Copy link
Contributor

We should probably handle this together with #223

@tripodsan
Copy link
Contributor

+1 for (1). having middleware operating on the req/res is quite common in the js world.

@tripodsan tripodsan mentioned this issue Apr 10, 2019
Closed
@kptdobe
Copy link
Contributor

kptdobe commented Apr 10, 2019 •
edited
Loading

+1 for (1). without building a complex and costly machinery, this is almost impossible to do anyway.

@rofe
Copy link
Contributor

rofe commented Apr 10, 2019

+1 for (1) for the reasons already stated

@koraa koraa mentioned this issue Apr 12, 2019
Merged
@trieloff trieloff added the bug Something isn't working label Apr 26, 2019
koraa added a commit that referenced this issue May 2, 2019
@koraa
BREAKING CHANGE: feat: Reimplement pipeline step executer
a73a192 
feat: Drop write protection in pipeline fixes #228
feat: Get rid of parameter merging fixes #223
feat: Fuller error reporting in pipeline

  Specifically, all errors are now reported by printing an
  error message the stack trace with additional information
  identifying the specific pipeline step.
  On the http level a 500 status code is transmitted.

fix: Get rid of bail() calls

  Bail calls basically hid the stack trace from the devs because
  they would just print an error message and send a 500 status code.
  By removing them and throwing exceptions instead we can make full
  use of the improved error reporting above.
koraa added a commit that referenced this issue May 3, 2019
@koraa
BREAKING CHANGE: feat: Reimplement pipeline step executer
13abe61 
feat: Drop write protection in pipeline fixes #228
feat: Get rid of parameter merging fixes #223
feat: Fuller error reporting in pipeline

  Specifically, all errors are now reported by printing an
  error message the stack trace with additional information
  identifying the specific pipeline step.
  On the http level a 500 status code is transmitted.

fix: Get rid of bail() calls

  Bail calls basically hid the stack trace from the devs because
  they would just print an error message and send a 500 status code.
  By removing them and throwing exceptions instead we can make full
  use of the improved error reporting above.
koraa added a commit that referenced this issue May 7, 2019
@koraa
BREAKING CHANGE: feat: Simplify pipeline step executer
8c20072 
feat: Drop write protection in pipeline fixes #228
feat: Get rid of parameter merging fixes #223
feat: Fuller error reporting in pipeline

  Specifically, all errors are now reported by printing an
  error message the stack trace with additional information
  identifying the specific pipeline step.
  On the http level a 500 status code is transmitted.

fix: Get rid of bail() calls

  Bail calls basically hid the stack trace from the devs because
  they would just print an error message and send a 500 status code.
  By removing them and throwing exceptions instead we can make full
  use of the improved error reporting above.
koraa added a commit that referenced this issue May 9, 2019
@koraa
BREAKING CHANGE: feat: Simplify pipeline step executer
3f8d08f 
feat: Drop write protection in pipeline fixes #228
feat: Get rid of parameter merging fixes #223
feat: Fuller error reporting in pipeline

  Specifically, all errors are now reported by printing an
  error message the stack trace with additional information
  identifying the specific pipeline step.
  On the http level a 500 status code is transmitted.

fix: Get rid of bail() calls

  Bail calls basically hid the stack trace from the devs because
  they would just print an error message and send a 500 status code.
  By removing them and throwing exceptions instead we can make full
  use of the improved error reporting above.

feat: Simplify pipeline error handling

  Can not longer clear errors
tripodsan pushed a commit that referenced this issue May 10, 2019
@koraa @tripodsan
feat(pipe): Simplify pipeline step executor
dae5487 
BREAKING CHANGE: return value from pipeline functions are no longer merged into the context

feat: Drop write protection in pipeline fixes #228
feat: Get rid of parameter merging fixes #223
feat: Fuller error reporting in pipeline

  Specifically, all errors are now reported by printing an
  error message the stack trace with additional information
  identifying the specific pipeline step.
  On the http level a 500 status code is transmitted.

fix: Get rid of bail() calls

  Bail calls basically hid the stack trace from the devs because
  they would just print an error message and send a 500 status code.
  By removing them and throwing exceptions instead we can make full
  use of the improved error reporting above.

feat: Simplify pipeline error handling

  Can not longer clear errors
koraa added a commit that referenced this issue May 10, 2019
@koraa
feat(pipe): Simplify pipeline step executor
a01ef8e 
BREAKING CHANGE: return value from pipeline functions are no longer merged into the context

Co-authored-by: Tobias Bocanegra <tripod@adobe.com>

NOTE: Most of the functional changes live in src/pipeline.js;
most other changes are just refactoring the rest of the code
base to utilize the changes there.

feat: Drop write protection in pipeline fixes #228
feat: Get rid of parameter merging fixes #223

  This specific change necessitated the bulk of the changes to the code
  bases; most of the code that was refactored relied on parameter
  merging before.

feat: Fuller error reporting in pipeline

  Specifically, all errors are now reported by printing an
  error message the stack trace with additional information
  identifying the specific pipeline step.
  On the http level a 500 status code is transmitted.

fix: Get rid of bail() calls

  Bail calls basically hid the stack trace from the devs because
  they would just print an error message and send a 500 status code.
  By removing them and throwing exceptions instead we can make full
  use of the improved error reporting above.
koraa added a commit that referenced this issue May 10, 2019
@koraa
feat(pipe): Simplify pipeline step executor
fe0cb12 
BREAKING CHANGE: return value from pipeline functions are no longer merged into the context

Co-authored-by: Tobias Bocanegra <tripod@adobe.com>

NOTE: Most of the functional changes live in src/pipeline.js;
most other changes are just refactoring the rest of the code
base to utilize the changes there.

feat: Drop write protection in pipeline fixes #228
feat: Get rid of parameter merging fixes #223

  This specific change necessitated the bulk of the changes to the code
  bases; most of the code that was refactored relied on parameter
  merging before.

feat: Fuller error reporting in pipeline

  Specifically, all errors are now reported by printing an
  error message the stack trace with additional information
  identifying the specific pipeline step.
  On the http level a 500 status code is transmitted.

fix: Get rid of bail() calls

  Bail calls basically hid the stack trace from the devs because
  they would just print an error message and send a 500 status code.
  By removing them and throwing exceptions instead we can make full
  use of the improved error reporting above.
koraa added a commit that referenced this issue May 10, 2019
@koraa @tripodsan
feat(pipe): Simplify pipeline step executor
26d4825 
BREAKING CHANGE: return value from pipeline functions are no longer merged into the context

NOTE: Most of the functional changes live in src/pipeline.js;
most other changes are just refactoring the rest of the code
base to utilize the changes there.

feat: Drop write protection in pipeline fixes #228
feat: Get rid of parameter merging fixes #223

  This specific change necessitated the bulk of the changes to the code
  bases; most of the code that was refactored relied on parameter
  merging before.

feat: Fuller error reporting in pipeline

  Specifically, all errors are now reported by printing an
  error message the stack trace with additional information
  identifying the specific pipeline step.
  On the http level a 500 status code is transmitted.

fix: Get rid of bail() calls

  Bail calls basically hid the stack trace from the devs because
  they would just print an error message and send a 500 status code.
  By removing them and throwing exceptions instead we can make full
  use of the improved error reporting above.

Co-authored-by: Tobias Bocanegra <tripod@adobe.com>
tripodsan added a commit that referenced this issue May 14, 2019
@koraa @tripodsan
feat(pipe): Simplify pipeline step executor
b709922 
BREAKING CHANGE: return value from pipeline functions are no longer merged into the context

NOTE: Most of the functional changes live in src/pipeline.js;
most other changes are just refactoring the rest of the code
base to utilize the changes there.

feat: Drop write protection in pipeline fixes #228
feat: Get rid of parameter merging fixes #223

  This specific change necessitated the bulk of the changes to the code
  bases; most of the code that was refactored relied on parameter
  merging before.

feat: Fuller error reporting in pipeline

  Specifically, all errors are now reported by printing an
  error message the stack trace with additional information
  identifying the specific pipeline step.
  On the http level a 500 status code is transmitted.

fix: Get rid of bail() calls

  Bail calls basically hid the stack trace from the devs because
  they would just print an error message and send a 500 status code.
  By removing them and throwing exceptions instead we can make full
  use of the improved error reporting above.

Co-authored-by: Tobias Bocanegra <tripod@adobe.com>
tripodsan added a commit that referenced this issue May 14, 2019
@koraa @tripodsan
feat(pipe): Simplify pipeline step executor
458bbc3 
BREAKING CHANGE: return value from pipeline functions are no longer merged into the context

NOTE: Most of the functional changes live in src/pipeline.js;
most other changes are just refactoring the rest of the code
base to utilize the changes there.

feat: Drop write protection in pipeline fixes #228
feat: Get rid of parameter merging fixes #223

  This specific change necessitated the bulk of the changes to the code
  bases; most of the code that was refactored relied on parameter
  merging before.

feat: Fuller error reporting in pipeline

  Specifically, all errors are now reported by printing an
  error message the stack trace with additional information
  identifying the specific pipeline step.
  On the http level a 500 status code is transmitted.

fix: Get rid of bail() calls

  Bail calls basically hid the stack trace from the devs because
  they would just print an error message and send a 500 status code.
  By removing them and throwing exceptions instead we can make full
  use of the improved error reporting above.

Co-authored-by: Tobias Bocanegra <tripod@adobe.com>
adobe-bot pushed a commit that referenced this issue May 16, 2019
@semantic-release-bot
chore(release): 2.0.0 [skip ci]
0ee79bc 
# [2.0.0](v1.12.1...v2.0.0) (2019-05-16)

### Bug Fixes

* **pipe:** Do not filter out non-functions before running the pipeline ([083d902](083d902))

### Features

* **pipe:** Simplify pipeline step executor ([2cbfe58](2cbfe58)), closes [#228](#228) [#223](#223)

### BREAKING CHANGES

* **pipe:** return value from pipeline functions are no longer merged into the context

NOTE: Most of the functional changes live in src/pipeline.js;
most other changes are just refactoring the rest of the code
base to utilize the changes there.
@adobe-bot
Copy link

🎉 This issue has been resolved in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Something isn't working released
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@trieloff @kptdobe @tripodsan @koraa @rofe @adobe-bot