Support external store #66

dead-horse · 2017-02-22T12:07:01Z

Feature

Support external store by pass options.store.
Throw when encode session error, consider a breaking change.
Clean cookie when decode session throw error, ensure next request won't throw again.

Fixes

Customize options.decode will check expired now.
Remove Semantics in README because it's not "guest" sessions any more.

Compatibility

All original test cases are passed.
Drop support for node < 4.
Internal implementations are changed, so some private API is changed.
- Change private api session.save(), won't set cookie immediately now.
- Remove private api session.changed().
- Remove undocumented property context.sessionKey, can use opts.key instead.
- Change undocumented property context.sessionOptions to getter.

Performance

QPS is reduced by <10% because of more generator function are used.

coveralls · 2017-02-22T12:09:53Z

Coverage decreased (-0.4%) to 93.956% when pulling bc9dc97 on external into 3bb2f0b on master.

coveralls · 2017-02-22T12:09:53Z

Coverage decreased (-0.4%) to 93.956% when pulling bc9dc97 on external into 3bb2f0b on master.

coveralls · 2017-02-22T12:09:53Z

Coverage decreased (-0.4%) to 93.956% when pulling bc9dc97 on external into 3bb2f0b on master.

dead-horse · 2017-02-22T12:10:38Z

maybe we can deprecate koa-generic-session after merge this, tow session modules with slightly different make user confused.

coveralls · 2017-02-22T12:15:46Z

Coverage increased (+0.6%) to 94.972% when pulling 369616e on external into 3bb2f0b on master.

fengmk2 · 2017-02-22T12:40:41Z

lib/session.js

+   */
+
+  constructor(ctx, obj) {
+    this._ctx = ctx;


ContextSession named ctx but here is _ctx, what's the different on naming ctx property?

I see. toJSON hacking

fengmk2

LGTM

fengmk2 · 2017-02-22T12:49:02Z

Should release a major version.

dead-horse · 2017-02-22T15:04:03Z

@fengmk2 add 490f31a

coveralls · 2017-02-22T15:11:01Z

Coverage increased (+5.6%) to 100.0% when pulling 490f31a on external into 3bb2f0b on master.

coveralls · 2017-02-22T15:13:02Z

Coverage increased (+5.6%) to 100.0% when pulling 4687a38 on external into 3bb2f0b on master.

fengmk2 · 2017-02-22T15:23:55Z

index.js

    } catch (err) {
      throw err;
    } finally {
-      commit(this, this._prevjson, this._sess, opts);
+      yield this.sess.commit();


~~commit will throw error and who will catch this error? Is it send error response twice when commit error and yield next error happen in the same time?~~

--- EDIT ---

It look ok on context.onerror even error throw twice https://github.com/koajs/koa/blob/master/lib/context.js#L113

fengmk2 · 2017-02-22T15:39:48Z

Need one more reviewer @popomore

dead-horse · 2017-02-22T15:39:51Z

lib/context.js

+      // when `string` is not base64-encoded.
+      // but `JSON.parse(string)` will crash.
+      debug('decode %j error: %s', cookie, err);
+      if (!(err instanceof SyntaxError)) throw err;


maybe we should clean this cookie to ensure next request won't thrown again.

but this error should rarely happend

If we don't help user to clean up, they won't never get the normal response.

coveralls · 2017-02-22T15:58:54Z

Coverage increased (+5.6%) to 100.0% when pulling ef64dcc on external into 3bb2f0b on master.

coveralls · 2017-02-22T15:59:59Z

Coverage increased (+5.6%) to 100.0% when pulling ef64dcc on external into 3bb2f0b on master.

coveralls · 2017-02-23T02:38:33Z

Coverage increased (+5.6%) to 100.0% when pulling 46ee6a5 on external into 3bb2f0b on master.

popomore · 2017-02-23T03:52:51Z

index.js

-  var body = new Buffer(string, 'base64').toString('utf8');
-  var json = JSON.parse(body);
+function extendContext(context, opts) {
+  context.__defineGetter__('sess', function() {


Why not use Object.defineProperty, this API is deprecated https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/__defineGetter__

Or can use assign?

Object.assign(context, { get sess() {}, })

popomore · 2017-02-23T03:56:01Z

index.js

-  var json = JSON.parse(body);
+function extendContext(context, opts) {
+  context.__defineGetter__('sess', function() {
+    if (this._sess) return this._sess;


We can use Symbol now?

popomore · 2017-02-23T04:00:42Z

lib/context.js

+      return;
+    }
+
+    const json = yield this.store.get(externalKey, ctx);


Will it throw?

ctx always be the first?

it could throw here, it is strong dependent on external store if options.store present.

I'll remove ctx here, it is useless. Keep the signature the same as koa-generic-session's store.

popomore · 2017-02-23T04:05:23Z

lib/context.js

+
+    // cookie session store
+    if (!this.store) this.initFromCookie();
+    return this.session;


Why just return session?

session is undefined here, this.session will be assigned after this.initFromCookie()

popomore · 2017-02-23T04:12:10Z

index.js

-    // make sessionOptions independent in each request
-    initSessionOptions(this, opts);
+  return function* session(next) {
+    if (this.sess.store) yield this.sess.initFromExternal();


Can we use cookie session as one of the store, so this needn't call external.

cookie store is a little different from other external stores.

it is synchronize so it can be lazy loading,

it use the same cookie field to store the value.

If we treat cookie session as a store, we still need to write some tricky code.

popomore · 2017-02-23T04:19:36Z

lib/context.js

+   * @api private
+   */
+
+  initFromCookie() {


initFromCookie will call when first get, and initFromExternal run in middleware

lazy loading is good for those route don't touch session when using cookie storage.

coveralls · 2017-02-23T06:07:56Z

Coverage increased (+5.6%) to 100.0% when pulling ffb15e5 on external into 3bb2f0b on master.

coveralls · 2017-02-23T06:18:35Z

Coverage increased (+5.6%) to 100.0% when pulling d1b2112 on external into 3bb2f0b on master.

coveralls · 2017-02-24T03:12:10Z

Coverage increased (+5.6%) to 100.0% when pulling 012283e on external into 3bb2f0b on master.

dead-horse · 2017-02-24T03:12:52Z

ping @popomore @jonathanong

coveralls · 2017-02-24T03:14:08Z

Coverage increased (+5.6%) to 100.0% when pulling e33923b on external into 3bb2f0b on master.

dead-horse · 2017-02-24T14:43:19Z

@jonathanong we gonna merge this PR if there is no new question until next Monday.

jonathanong · 2017-02-25T05:21:17Z

@dead-horse np

coveralls · 2017-02-27T13:00:09Z

Coverage increased (+5.6%) to 100.0% when pulling f916f20 on external into 3bb2f0b on master.

coveralls · 2017-02-27T13:00:09Z

Coverage increased (+5.6%) to 100.0% when pulling f916f20 on external into 3bb2f0b on master.

coveralls · 2017-02-27T13:00:09Z

Coverage increased (+5.6%) to 100.0% when pulling f916f20 on external into 3bb2f0b on master.

dead-horse · 2017-02-27T13:08:09Z

I'll merge this and release a new major version, and then rewrite it with async function for koa 2 support.

dead-horse added 10 commits February 20, 2017 18:38

refactor: support external

5c46711

feat: support remove from external

9c9c815

refactor: seperate session model

44f6b53

refactor: use context session

e34d3db

fix: files add lib

c7c9b33

test: remove 0.12, add 7

ec44f96

test: add external test cases and eslint

83bb001

chore: eslint

b748403

refactor: use store instead of external

54576d0

refactor: rename methods

bc9dc97

dead-horse added the enhancement label Feb 22, 2017

dead-horse requested review from fengmk2, popomore, tj and jonathanong February 22, 2017 12:07

chore: remove unused code

369616e

fengmk2 reviewed Feb 22, 2017

View reviewed changes

fengmk2 approved these changes Feb 22, 2017

View reviewed changes

feat: throw error when encode error, add more test cases

490f31a

chore: remove unused code

4687a38

fengmk2 reviewed Feb 22, 2017

View reviewed changes

dead-horse commented Feb 22, 2017

View reviewed changes

dead-horse added 2 commits February 22, 2017 23:56

feat: clean cookie when decode error

66ca6e3

test: remove harmony

ef64dcc

jonathanong self-assigned this Feb 22, 2017

feat: keep session.save()

46ee6a5

popomore reviewed Feb 23, 2017

View reviewed changes

refactor: use defineProperties and symbol

ffb15e5

docs: declare only use external session stores if necessary

d1b2112

dead-horse mentioned this pull request Feb 23, 2017

1.0 Milestone eggjs/egg#5

Closed

31 tasks

fix: typo

012283e

deps: remove deep-equal

e33923b

popomore approved these changes Feb 27, 2017

View reviewed changes

feat: save session when maxAge changed

f916f20

dead-horse merged commit 2f7010b into master Feb 27, 2017

dead-horse deleted the external branch February 27, 2017 13:08

Support external store #66

Support external store #66

Conversation

dead-horse commented Feb 22, 2017 • edited Loading

Feature

Fixes

Compatibility

Performance

coveralls commented Feb 22, 2017

coveralls commented Feb 22, 2017

coveralls commented Feb 22, 2017

dead-horse commented Feb 22, 2017 • edited Loading

coveralls commented Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fengmk2 left a comment

Choose a reason for hiding this comment

fengmk2 commented Feb 22, 2017

dead-horse commented Feb 22, 2017

coveralls commented Feb 22, 2017 • edited Loading

coveralls commented Feb 22, 2017 • edited Loading

fengmk2 Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

fengmk2 commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 22, 2017 • edited Loading

coveralls commented Feb 22, 2017 • edited Loading

coveralls commented Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 23, 2017 • edited Loading

coveralls commented Feb 23, 2017 • edited Loading

coveralls commented Feb 24, 2017 • edited Loading

dead-horse commented Feb 24, 2017

coveralls commented Feb 24, 2017 • edited Loading

dead-horse commented Feb 24, 2017

jonathanong commented Feb 25, 2017

coveralls commented Feb 27, 2017

coveralls commented Feb 27, 2017

coveralls commented Feb 27, 2017

dead-horse commented Feb 27, 2017

dead-horse commented Feb 22, 2017 •

edited

Loading

dead-horse commented Feb 22, 2017 •

edited

Loading

coveralls commented Feb 22, 2017 •

edited

Loading

coveralls commented Feb 22, 2017 •

edited

Loading

coveralls commented Feb 22, 2017 •

edited

Loading

fengmk2 Feb 22, 2017 •

edited

Loading

coveralls commented Feb 22, 2017 •

edited

Loading

coveralls commented Feb 22, 2017 •

edited

Loading

coveralls commented Feb 23, 2017 •

edited

Loading

coveralls commented Feb 23, 2017 •

edited

Loading

coveralls commented Feb 23, 2017 •

edited

Loading

coveralls commented Feb 24, 2017 •

edited

Loading

coveralls commented Feb 24, 2017 •

edited

Loading