crypto: simplify Certificate class bindings #5382

estliberitas · 2016-02-23T12:51:12Z

Description of change

Replace Certificate C++ class with simple functions. Update
crypto.Certificate methods accordingly.

Original talk was in comments of #5230.

I removed Certificate C++ class from crypto bindings and replaced its methods with simple functions. I also prefixed function names with cert in order to separate these functions from others like randomBytes, etc. (to be discussed).

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

crypto

Replace Certificate C++ class with simple functions. Update crypto.Certificate methods accordingly.

estliberitas · 2016-02-23T12:51:56Z

/cc @bnoordhuis

bnoordhuis · 2016-02-23T15:45:00Z

LGTM. Landing it will have to wait a while until we can run the CI again, see #5194 (comment).

estliberitas · 2016-02-23T15:50:10Z

@bnoordhuis np

estliberitas · 2016-02-27T18:50:31Z

@bnoordhuis I guess CI is alive already? 😉

bnoordhuis · 2016-02-28T21:53:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1774/

jasnell · 2016-03-02T17:43:24Z

LGTM /cc @nodejs/crypto

bnoordhuis · 2016-03-05T10:57:50Z

One more time for good luck: https://ci.nodejs.org/job/node-test-pull-request/1855/

bnoordhuis · 2016-03-05T11:31:05Z

All green. Thanks, landed in a37401e.

Replace Certificate C++ class with simple functions. Update crypto.Certificate methods accordingly. PR-URL: #5382 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

bnoordhuis · 2016-03-05T11:31:52Z

@jasnell I'll leave it up to you if you want to cherry-pick this into v4 and v5.

jasnell · 2016-03-07T18:02:43Z

Picking into v5 should be possible but it wouldn't land in v4. /cc @rvagg @thealphanerd

Replace Certificate C++ class with simple functions. Update crypto.Certificate methods accordingly. PR-URL: #5382 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Fishrock123 · 2016-03-08T20:02:44Z

lib/crypto.js

@@ -592,23 +592,21 @@ exports.Certificate = Certificate;
 function Certificate() {
  if (!(this instanceof Certificate))
    return new Certificate();
-
-  this._handle = new binding.Certificate();


@bnoordhuis / @jasnell This is tagged as semver-minor but that seems incorrect; this looks like a major?

We tagged it semver-minor because it's cleanup, not a bug fix. The only thing it does is replace the (superfluous) _handle private property for direct binding calls.

Fishrock123 · 2016-03-08T20:50:40Z

Ok well I'm not comfortable with this so I'm pulling the trigger and removing it from v5.x

jasnell · 2016-03-08T21:05:01Z

Works for me.

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

crypto: simplify Certificate class bindings

6758edc

Replace Certificate C++ class with simple functions. Update crypto.Certificate methods accordingly.

bnoordhuis added crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Feb 23, 2016

bnoordhuis closed this Mar 5, 2016

bnoordhuis pushed a commit that referenced this pull request Mar 5, 2016

crypto: simplify Certificate class bindings

a37401e

Replace Certificate C++ class with simple functions. Update crypto.Certificate methods accordingly. PR-URL: #5382 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Fishrock123 mentioned this pull request Mar 8, 2016

v5.8.0 proposal #5559

Merged

Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016

crypto: simplify Certificate class bindings

c7ebef7

Replace Certificate C++ class with simple functions. Update crypto.Certificate methods accordingly. PR-URL: #5382 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Fishrock123 reviewed Mar 8, 2016
View reviewed changes

Fishrock123 added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 8, 2016

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

jasnell mentioned this pull request Apr 19, 2016

What is new in v6? #6264

Closed

jasnell mentioned this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release #6383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

crypto: simplify Certificate class bindings #5382

crypto: simplify Certificate class bindings #5382

Uh oh!

estliberitas commented Feb 23, 2016

Uh oh!

estliberitas commented Feb 23, 2016

Uh oh!

bnoordhuis commented Feb 23, 2016

Uh oh!

estliberitas commented Feb 23, 2016

Uh oh!

estliberitas commented Feb 27, 2016

Uh oh!

bnoordhuis commented Feb 28, 2016

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

bnoordhuis commented Mar 5, 2016

Uh oh!

bnoordhuis commented Mar 5, 2016

Uh oh!

bnoordhuis commented Mar 5, 2016

Uh oh!

jasnell commented Mar 7, 2016

Uh oh!

Fishrock123 Mar 8, 2016

Uh oh!

bnoordhuis Mar 8, 2016

Uh oh!

Fishrock123 commented Mar 8, 2016

Uh oh!

jasnell commented Mar 8, 2016

Uh oh!

Uh oh!

Uh oh!

crypto: simplify Certificate class bindings #5382

crypto: simplify Certificate class bindings #5382

Uh oh!

Conversation

estliberitas commented Feb 23, 2016

Description of change

Pull Request check-list

Affected core subsystem(s)

Uh oh!

estliberitas commented Feb 23, 2016

Uh oh!

bnoordhuis commented Feb 23, 2016

Uh oh!

estliberitas commented Feb 23, 2016

Uh oh!

estliberitas commented Feb 27, 2016

Uh oh!

bnoordhuis commented Feb 28, 2016

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

bnoordhuis commented Mar 5, 2016

Uh oh!

bnoordhuis commented Mar 5, 2016

Uh oh!

bnoordhuis commented Mar 5, 2016

Uh oh!

jasnell commented Mar 7, 2016

Uh oh!

Fishrock123 Mar 8, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Mar 8, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Mar 8, 2016

Uh oh!

jasnell commented Mar 8, 2016

Uh oh!

Uh oh!