Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New api #1

Merged
merged 16 commits into from
Nov 13, 2018
Merged

New api #1

merged 16 commits into from
Nov 13, 2018

Conversation

binarymist
Copy link
Contributor

@binarymist binarymist commented Oct 29, 2018

  • The apiKey is now required on ZapClient instantiation only. By default the apiKey is required by Zap
  • Support added for promises and legacy callbacks
  • Replaced vars with consts
  • Generation should be based on develop branch and from the latest minor tag (I.E. 2.7.0) to not include unreleased API endpoints
  • Tested on my projects which leverage promises

Addresses #5044

src/index.js Outdated Show resolved Hide resolved
@thc202
Copy link
Member

thc202 commented Oct 29, 2018

Was the API generated from develop branch? Ideally it should be from tag 2.7.0 to not include unreleased API endpoints.

@binarymist
Copy link
Contributor Author

Tested:

  • Running Zap with -config api.disablekey=true and instantiating ClientApi with:
    1. Good key: subsequent requests succeed
    2. Bad key: subsequent requests succeed
  • Running Zap with -config api.key=<key> and instantiating ClientApi with:
    1. Good key: subsequent requests succeed
    2. Bad key: subsequent requests fail

@binarymist
Copy link
Contributor Author

binarymist commented Oct 30, 2018

API was generated from develop branch, not tag 2.7.0, wasn't aware of this requirement. See (zaproxy/zaproxy#5088 (comment))

@binarymist
Copy link
Contributor Author

Was the API generated from develop branch? Ideally it should be from tag 2.7.0

8fdfeae

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@binarymist
Copy link
Contributor Author

Thanks for the review @thc202

@binarymist
Copy link
Contributor Author

Breaking while dog fooding until extensions removed:
16225c9

@binarymist
Copy link
Contributor Author

How's it going @thc202 @psiinon ? What can I do to help push this through?

@psiinon
Copy link
Member

psiinon commented Nov 3, 2018

It its ready to review then we can look at it next week :)
Many thanks!

@binarymist
Copy link
Contributor Author

With the week being nearly over, I thought I'd ask how's progress?

@thc202
Copy link
Member

thc202 commented Nov 7, 2018

I talked with @psiinon yesterday about this, I'll push the add-ons' APIs shortly (with that it should be good to go?).

@binarymist
Copy link
Contributor Author

It'd be good if I can do all of that generation as well, to help take load off of you. As the Node API maintainer I'm happy to do all of that each time, so long as I know what to do. I've been recording all my steps so far, so this should just become part of that procedure. Once the additional add-ons are generated -> commited -> pushed (to this PR I guess?), then I can go through all of the API files for a review.

These todo's are all being tracked here

@thc202
Copy link
Member

thc202 commented Nov 7, 2018

Agreed, but the API generation is currently broken in the zap-extensions repo, that's why I'm suggesting pushing/doing that myself (it will require a newer core version to fix it, 2.8.0?). Yes, it will be pushed to this PR.

@thc202
Copy link
Member

thc202 commented Nov 8, 2018

@binarymist APIs pushed, PTAL (feel free to squash/fix up, if it looks fine). I'll open PRs to zap-extensions to fix the generation (it will still require a new ZAP jar for proper generation though).

@binarymist
Copy link
Contributor Author

Roger that @thc202, and thanks! Will try review this weekend.

@binarymist
Copy link
Contributor Author

binarymist commented Nov 11, 2018

I've checked about 2/3 of your files @thc202, and a small collection of mine and I'm not seeing any obvious issues.

There are some tweaks I'd like to make at some stage that are not essential, like changing the line:

out.write("  if ("+safeName(param.toLowerCase())+" && "+safeName(param.toLowerCase())+" !== null) {\n");

where if (start) would be sufficient, the !== null check seems redundant.

Example in ajaxSpider.js and many other files:

if (start && start !== null)

It'd be good to get this PR merged first though and ideally published to NPM. Where should we track these types of changes, I'd like to assign them to me, but I won't see them on this repo.

If you've had a bit of a look through the files, then I guess it's just a couple more items to check off?

@binarymist
Copy link
Contributor Author

How are we going with this @psiinon @thc202 ?

README.md Outdated Show resolved Hide resolved
@thc202
Copy link
Member

thc202 commented Nov 12, 2018

Good to go once the typo is fixed.

@thc202
Copy link
Member

thc202 commented Nov 12, 2018

Regarding the planned tweaks, I think it's fine to raise the issues in the main repo.

@binarymist
Copy link
Contributor Author

Regarding the planned tweaks, I think it's fine to raise the issues in the main repo.

Ok, but someone will need to bug me about them, else I may forget.

@thc202
Copy link
Member

thc202 commented Nov 13, 2018

Sure.

Thanks for the PR!

@kingthorin
Copy link
Member

kingthorin commented Nov 13, 2018

Ok, but someone will need to bug me about them, else I may forget.

Can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants