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

fix: objectId casting now working #111

Merged
merged 15 commits into from
Jun 30, 2023
Merged

fix: objectId casting now working #111

merged 15 commits into from
Jun 30, 2023

Conversation

giuliowaitforitdavide
Copy link
Contributor

@giuliowaitforitdavide giuliowaitforitdavide commented Jun 23, 2023 •
edited
Loading

Pull Request Type

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

Description

The getList was returing an error Cannot read properties of undefined, reading .toString() when try to cast ObjectIds

PR Checklist

  • The commit message follows our guidelines included in the CONTRIBUTING.md
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Does this PR introduce a breaking change?

  • Yes
  • No

ThisIsDemetrio
ThisIsDemetrio approved these changes Jun 23, 2023
View reviewed changes
Copy link
Collaborator

@ThisIsDemetrio ThisIsDemetrio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

hiimjako
hiimjako suggested changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@hiimjako hiimjako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems fine, but if you try to look at the generated swagger you will see that it some fields it shows: Unknown Type: string,object or Unknown Type: array,string.
Can you try to give it a look? It seems an error related to JSONschemaGenerator and/or ajv configuration.

@danibix95 danibix95 requested a review from hiimjako June 23, 2023 14:35
@danibix95
Copy link
Collaborator

It seems fine, but if you try to look at the generated swagger you will see that it some fields it shows: Unknown Type: string,object or Unknown Type: array,string. Can you try to give it a look? It seems an error related to JSONschemaGenerator and/or ajv configuration.

@giuliowaitforitdavide

@giuliowaitforitdavide
Copy link
Contributor Author

It seems fine, but if you try to look at the generated swagger you will see that it some fields it shows: Unknown Type: string,object or Unknown Type: array,string. Can you try to give it a look? It seems an error related to JSONschemaGenerator and/or ajv configuration.

Fixed

@ThisIsDemetrio ThisIsDemetrio self-requested a review June 27, 2023 15:51
@hiimjako
Copy link
Contributor

Still have the same problem on ObjectId, Date and Array of object field. The fix should be something similar to the previous ones.

hiimjako
hiimjako approved these changes Jun 30, 2023
View reviewed changes
Copy link
Contributor

@hiimjako hiimjako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good job!

@giuliowaitforitdavide giuliowaitforitdavide merged commit abe99e1 into main Jun 30, 2023
@giuliowaitforitdavide giuliowaitforitdavide deleted the fix/lookup-validation branch June 30, 2023 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThisIsDemetrio ThisIsDemetrio ThisIsDemetrio approved these changes

@hiimjako hiimjako hiimjako approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@giuliowaitforitdavide @danibix95 @hiimjako @ThisIsDemetrio