Page MenuHomePhabricator

[dagster] whitelisted namedtuple serdes
ClosedPublic

Authored by alangenfeld on Aug 16 2019, 5:30 PM.

Details

Reviewers
schrockn
Group Reviewers
Restricted Project
Commits
R1:0de98e61b1d8: [dagster] whitelisted namedtuple serdes
Summary

A scheme for serializing & deserializing dagster event data to and from json.

These are all namedtuples so by default you end up with lists if you just do default json
serialization. Even if you manage to encode them as dicts, you still dont know the classes
to use at deserialization time unless we manually maintain a bunch of code.

To resolve this, this serdes encodes the class name as __class__ then uses a predfined
whitelist to grab the actual class object using the name. I think this scheme prevents this
from being a security problem.

Test Plan

Unit test that round trips everything from the many_events pipeline.

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

alangenfeld created this revision.Aug 16 2019, 5:30 PM
alangenfeld planned changes to this revision.Aug 16 2019, 5:45 PM
alangenfeld added inline comments.
python_modules/dagster/dagster/core/events/serdes.py
26–51 ↗(On Diff #3768)

I tried to come up with a way to register these at the class creation callsite but couldnt come up with anything that worked - short of a whitelist(klass) call after defining it which didnt feel great.

alangenfeld added inline comments.
python_modules/dagster/dagster/core/events/serdes.py
26–51 ↗(On Diff #3768)

decorator

thanks @prha

alangenfeld updated this revision to Diff 3769.Aug 16 2019, 6:50 PM

use decorator

alangenfeld updated this revision to Diff 3773.Aug 16 2019, 8:31 PM

rebase w/ fixes

alangenfeld updated this revision to Diff 3782.Aug 16 2019, 9:22 PM

hold connection

alangenfeld updated this revision to Diff 3784.Aug 16 2019, 9:52 PM

open and close connections every time

schrockn accepted this revision.Mon, Aug 19, 8:28 PM
schrockn added a subscriber: schrockn.

decorator is nice

python_modules/dagster/dagster/core/serdes/__init__.py
2

i'm fine with rolling our own serialization system but

  1. this is probably worthy of a docblock
  2. esp why pickle or some other vetted serialization is insufficient or inappropriate for our use case.
This revision is now accepted and ready to land.Mon, Aug 19, 8:28 PM
This revision was automatically updated to reflect the committed changes.