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.

Sign up for GitHub

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

Jump to bottom

[Feature] Provide more supported traits #174

Closed
prince-chrismc opened this issue Sep 18, 2021 · 2 comments · Fixed by #193
Closed

[Feature] Provide more supported traits #174

prince-chrismc opened this issue Sep 18, 2021 · 2 comments · Fixed by #193
Assignees
@prince-chrismc
Labels
enhancement help wanted
Milestone
0.6.0

Comments

@prince-chrismc
Copy link
Collaborator

Currently we only offer the picojson_traits that is also the default.

It would be ideal to provide all the known support traits

  1. It's better for consumers
  2. It's less duplication
  3. easier to maintain

The proposal would be to have a header file for each support library

include/
├─ jwt-cpp/
│  ├─ jwt.h
│  ├─ base.h
│  ├─ traits/
│  │  ├─ picojson.h
│  │  ├─ nlohmann_json.h
│  │  ├─ jsoncons.h
│  │  ├─ boost_json.h

❓ Do we provide default specializations? This requires a define to make sure there's only one included
❓ Does this require a CMake option?
❓ What about badges for tested version?
@prince-chrismc prince-chrismc added this to the 0.6.0 milestone Sep 18, 2021
@prince-chrismc prince-chrismc self-assigned this Sep 18, 2021
prince-chrismc added a commit to prince-chrismc/jwt-cpp that referenced this issue Sep 27, 2021
@prince-chrismc
clean up traits implementation
3a2d76d 
in prep for Thalhammer#174
prince-chrismc added a commit that referenced this issue Sep 27, 2021
@prince-chrismc
⬆️ Bump danielaparker/jsoncons to v0.167.1 (#178)
ab1a60e 
* Update traits.yml

* Update traits.yml

* better error report for missing method

* clean up traits implementation

in prep for #174
prince-chrismc added a commit to prince-chrismc/jwt-cpp that referenced this issue Sep 27, 2021
@prince-chrismc
quick poc for Thalhammer#174
083176b
@Thalhammer
Copy link
Owner

Thalhammer commented Sep 30, 2021 •
edited by prince-chrismc
Loading

Do we provide default specializations? This requires a define to make sure there's only one included

I don't think we can pull out the default support for picojson without breaking existing code. If so we would have to atleast make it optional (i.e. a compile options thats off by default). Or wait a version and mark it deprecated for a while.

Does this require a CMake option?

As long as we only add extra headers and stuff in there I dont think it needs one. If you dont include the header nothing changes and if you do, its pretty obvious that you need the respective json library available.

What about badges for tested version?

Shouldn't be a big issue, since we wouldn't need to test every combination between ssl and json. Testing every json against a single ssl implementation should be fine, as long as the ssl version does not break.

@prince-chrismc
Copy link
Collaborator Author

I don't think we can pull out the default support for picojson without breaking existing code.

I agree! thus far I have been able to maintain it as the default with the same cmake/compile option

As long as we only add extra headers and stuff in there I dont think it needs one.

The challenge is when the consumer links to multiple defaults then they will get incorrect results (since it creates the same symbols and links to the work json implementation)

As for file structures, I have this:

include/
├─ jwt-cpp/
│  ├─ jwt.h
│  ├─ base.h
│  ├─ traits/
│  │  ├─ kazuho-picojson/
│  │  │  ├─ traits.h
│  │  │  ├─ defaults.h
│  │  ├─ nlohmann-json/
│  │  │  ├─ traits.h
│  │  │  ├─ defaults.h
│  │  ├─ danielaparker-jsoncons/
│  │  │  ├─ traits.h
│  │  │  ├─ defaults.h
│  │  ├─ boost-json/
│  │  │  ├─ traits.h
│  │  │  ├─ defaults.h

I opted for a seperate defaults file so

  • multiple traits could be used if desired
  • the defaults could be auto generated ref

@prince-chrismc prince-chrismc mentioned this issue Dec 23, 2021
Merged
prince-chrismc added a commit that referenced this issue Jan 17, 2022
@prince-chrismc @kleinmrk
🚢 Provide trait implementation for supported JSON libraries (#193)
5c9787e 
* quick poc for #174

* Copy paste

* Rename include/jwt-cpp/traits/traits.h to include/jwt-cpp/traits/danielaparker-jsoncons/traits.h

* Create traits.h

* adding a namespace

* mustache + action to automate defaults generation

since they should always be the same

* fix order

* traits tests should use traits

* playing with header order

to make clang happy

* install all possible traits 

switch badge name to sha

* Update CMakeLists.txt

* Update traits.h

* try commenting corrected code

* rm typescript

* make sure to implace edit

* dont comment when no diff

* apply linting

* fixup boost-json traits

* move comment and render to composite action

* actions/github-script does not leave context in the usual spot

* enabled globstar

* make sure to forward inputs

* try using env normally

* fix typo

* pass in linter name correctly

* debug print

* fix linting issues

* more debug printing

* fix linting issues

* fix typpo + more rebug print

* apply linting

* more debug

* try npm install global to be less intrusive

* wip render traits tests

* Update lint.yml

* global install does not work

* Update action.yml

* Update defaults.h

* remove nodejs action poc

* include jsoncons in main test application (for coverage of new files)

* do not exclude json (since it's used by all the traits)

* exclude bundled json libraries

* apply linting

* add jsoncons for coverage testing

* jsoncons set default to a viable version

* exploring ci failure

* Revert "exploring ci failure" it's overlapping symbols

This reverts commit 3d5b7f9.

* add missing specialization

* be explicit with the tests

* Create CMakeLists.txt

* Create danielaparker-jsoncons.cpp

* Update lint.yml

* Update traits.yml

* lint

* better access too decoded claims

* more tests on verify

* tweak test

trying to find minimum

* lets enable linting tests

* using env impl for github script

* fix typo

* apply linting

* add boost to the tests (very hacky)

* Update lint.yml

* ceate light boost installer

there is no cmake install anymore so just a blanket copy should do

* remove path requirement

* outline for commit and push linting on comment

* try different webhook target

* more event types

* fix if for more event types

* just a test commit

* Update lint.yml

* Update lint.yml

* Update lint.yml

* Apply automatic changes

* add token

* screw up formatting

* Update lint.yml

* Update lint.yml

* Apply automatic changes

* fix merge conflicts

* missing errors

* playing with globbing

* lint issues

* matrix on comment should give many pushes

* Apply automatic changes

* Update lint.yml

* Apply automatic changes

* Apply automatic changes

* fix rebase

* apply render tests

* Apply automatic changes

* Apply automatic changes

* Delete package-lock.json

* fail linting

* Update .gitignore

* Update lint.yml

* Update lint.yml

* Update lint.yml

* Apply automatic changes

* Update lint.yml

* Update CMakeLists.txt

* refactor boost.json inclusion

- find paths
- compile library
- code coverage
- traits example

* Apply automatic changes

* debug log

* Update action.yml

* debug output

* Update action.yml

* Delete private-boost-json.cpp.in

* Update private-find-boost-json.cmake

* Update CMakeLists.txt

* Update CMakeLists.txt

* Create CMakeLists.txt

* Update boost-json.cpp

* Update boost-json.cpp

* Update boost-json.cpp

* Update boost-json.cpp

* remove old boost tests

* examples to include traits

* fix compilation

* test nlohmann json traits

* linting

* refactor traits to make badges

* try with less expression syntax

* trying single quotes

* advertise traits support

* Update lint.yml

* Update lint.yml

* Update lint.yml

* sync (prince-chrismc#16)

* 🔧 Replace RSA related deprecated functions (#189)

* RSA_size() --> EVP_PKEY_size()

* getting rid of deprecated functions in rsa-pss signature creation

* getting rid of deprecated functions in rsa-pss signature verification

* generate_hash not used in pss

* Update lint.yml

* Update lint.yml

* Update lint.yml

Co-authored-by: Marek Klein <kleinmrk@gmail.com>

* Update lint.yml

* make sure to use install `nlohmann-json`

* make action folder match trait names

* add example + badge for kazuho-picojson

* link to new examples

* revert debug code

* dont mix matrix with uses

* fix bad copy paste

* spec interface library so it's no compiled

* fix installer

* fix to interface

* fix example

* attempt 2

* make sure to avoid circular reference

* attempt 3

* white space

* white space

* Update and rename .github/actions/comment-linting/action.yml to .github/actions/process-linting-results/action.yml

* Update lint.yml

* Update action.yml

* Update action.yml

* Update README.md

Co-authored-by: prince-chrismc <prince-chrismc@users.noreply.github.com>
Co-authored-by: Marek Klein <kleinmrk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@prince-chrismc prince-chrismc

Labels
enhancement help wanted
Projects
None yet
Milestone
0.6.0
Development

Successfully merging a pull request may close this issue.

Provide trait implementation for supported JSON libraries prince-chrismc/jwt-cpp
2 participants
@Thalhammer @prince-chrismc