[Javascript] Fix for constructors not handling required fields with default values well - #6649

Merged
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default
Aug 20, 2020
Merged

[Javascript] Fix for constructors not handling required fields with default values well#6649
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default

Conversation

@tray2100

Copy link
Copy Markdown
Contributor

This PR is for issue #6648. I verified that the output was as expected where for a spec that contains a required field with a default value will now look like

_this['field']=field||'defaultValue';

instead of just

_this['field']=field;

where it forces the required field to be undefined even though we have a default value. I also noticed that the samples were out of date and there wasn't a generate script for ES5 so I've added those here. I can squash the commits if needed before merging.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc: @CodeNinjai@frol@cliffano

@wing328

Copy link
Copy Markdown
Member

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

@tray2100

Copy link
Copy Markdown
ContributorAuthor

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

Hey @wing328

I probably should have shared more of my use case. I've defined a oneOf entity and made the discriminator required in the schemas referenced in it. It feels super awkward right now when using the constructFromObject mechanism because you'll have to manually add in the discriminator after the object is constructed - even if you used the entity created by the generator in the first place. It's even more awkward if there is a chain of constructFromObject calls and the entity with the discriminator is a few levels down the object graph.

Setting the field to the default value if nothing is specified solves this problem in a minimalist way.

This is essentially the spec:

 OneOfDogCat:
oneOf:
- $ref: "#/components/schemas/Dog"
- $ref: "#/components/schemas/Cat"
type: "object"
discriminator:
propertyName: "entityType"
Dog:
type: "object"
properties:
entityType:
default: "dog"
type: "string"
name:
type: "string"
required:
- "entityType"
Cat:
type: "object"
properties:
entityType:
default: "cat"
type: "string"
name:
type: "string"
required:
- "entityType"

…values
for required fields better. This change ensures the required field isn't
overridden with undefined when the object is constructed - especially through
a chain of constructFromObject calls..
@wing328

Copy link
Copy Markdown
Member

@tray2100 as discussed we can use {{#oneOf}} .. {{/oneOf}} to further enhance this fix for oneOf models only. Let's see if the community can help out with this.

@wing328
wing328 merged commit 71321bd into OpenAPITools:masterAug 20, 2020
@wing328wing328 added this to the 5.0.0 milestone Aug 20, 2020
jimschubert added a commit that referenced this pull request Aug 23, 2020
* master: (720 commits)
[docs] Update README badges (#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue #7258 fix (#7259)
[Dart] Get all enum values in a list (#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (#7273)
[core][bug] FILES is now path relative with no prefixes (#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (#7268)
[Java][jersey2] Add jersey injection dependencies (#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (#7255)
Date format dart (#6389)
minor enhancement to java client generator (#7253)
typescript: Fix Union Types Import Issue (#6789)
Modifying the es5 and es6 templates for javascript to handle default values (#6649)
[python-exp] simplify examples (#7157)
Support for KumuluzEE microprofile runtime (#5944)
[C#][netcore] minor improvements and bug fixes (#7244)
Deprecate Flash (ActionScript) client generator (#7231)
...
jimschubert added a commit to mohamedelhabib/openapi-generator that referenced this pull request Aug 24, 2020
* master: (219 commits)
[java] Appropriate instantiation of model with dynamic properties (OpenAPITools#6052)
[docs] Update README badges (OpenAPITools#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue OpenAPITools#7258 fix (OpenAPITools#7259)
[Dart] Get all enum values in a list (OpenAPITools#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (OpenAPITools#7273)
[core][bug] FILES is now path relative with no prefixes (OpenAPITools#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (OpenAPITools#7268)
[Java][jersey2] Add jersey injection dependencies (OpenAPITools#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (OpenAPITools#7255)
Date format dart (OpenAPITools#6389)
minor enhancement to java client generator (OpenAPITools#7253)
typescript: Fix Union Types Import Issue (OpenAPITools#6789)
Modifying the es5 and es6 templates for javascript to handle default values (OpenAPITools#6649)
[python-exp] simplify examples (OpenAPITools#7157)
Support for KumuluzEE microprofile runtime (OpenAPITools#5944)
[C#][netcore] minor improvements and bug fixes (OpenAPITools#7244)
...
@wing328wing328 changed the title [Javascript] Fix for constructors not handling required fields with default values well #6648[Javascript] Fix for constructors not handling required fields with default values wellSep 2, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tray2100@wing328@ybelenko
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[Javascript] Fix for constructors not handling required fields with default values well - #6649

Merged
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default
Aug 20, 2020
Merged

[Javascript] Fix for constructors not handling required fields with default values well#6649
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default

Conversation

@tray2100

Copy link
Copy Markdown
Contributor

This PR is for issue #6648. I verified that the output was as expected where for a spec that contains a required field with a default value will now look like

_this['field']=field||'defaultValue';

instead of just

_this['field']=field;

where it forces the required field to be undefined even though we have a default value. I also noticed that the samples were out of date and there wasn't a generate script for ES5 so I've added those here. I can squash the commits if needed before merging.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc: @CodeNinjai@frol@cliffano

@wing328

Copy link
Copy Markdown
Member

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

@tray2100

Copy link
Copy Markdown
ContributorAuthor

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

Hey @wing328

I probably should have shared more of my use case. I've defined a oneOf entity and made the discriminator required in the schemas referenced in it. It feels super awkward right now when using the constructFromObject mechanism because you'll have to manually add in the discriminator after the object is constructed - even if you used the entity created by the generator in the first place. It's even more awkward if there is a chain of constructFromObject calls and the entity with the discriminator is a few levels down the object graph.

Setting the field to the default value if nothing is specified solves this problem in a minimalist way.

This is essentially the spec:

 OneOfDogCat:
oneOf:
- $ref: "#/components/schemas/Dog"
- $ref: "#/components/schemas/Cat"
type: "object"
discriminator:
propertyName: "entityType"
Dog:
type: "object"
properties:
entityType:
default: "dog"
type: "string"
name:
type: "string"
required:
- "entityType"
Cat:
type: "object"
properties:
entityType:
default: "cat"
type: "string"
name:
type: "string"
required:
- "entityType"

…values
for required fields better. This change ensures the required field isn't
overridden with undefined when the object is constructed - especially through
a chain of constructFromObject calls..
@wing328

Copy link
Copy Markdown
Member

@tray2100 as discussed we can use {{#oneOf}} .. {{/oneOf}} to further enhance this fix for oneOf models only. Let's see if the community can help out with this.

@wing328
wing328 merged commit 71321bd into OpenAPITools:masterAug 20, 2020
@wing328wing328 added this to the 5.0.0 milestone Aug 20, 2020
jimschubert added a commit that referenced this pull request Aug 23, 2020
* master: (720 commits)
[docs] Update README badges (#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue #7258 fix (#7259)
[Dart] Get all enum values in a list (#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (#7273)
[core][bug] FILES is now path relative with no prefixes (#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (#7268)
[Java][jersey2] Add jersey injection dependencies (#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (#7255)
Date format dart (#6389)
minor enhancement to java client generator (#7253)
typescript: Fix Union Types Import Issue (#6789)
Modifying the es5 and es6 templates for javascript to handle default values (#6649)
[python-exp] simplify examples (#7157)
Support for KumuluzEE microprofile runtime (#5944)
[C#][netcore] minor improvements and bug fixes (#7244)
Deprecate Flash (ActionScript) client generator (#7231)
...
jimschubert added a commit to mohamedelhabib/openapi-generator that referenced this pull request Aug 24, 2020
* master: (219 commits)
[java] Appropriate instantiation of model with dynamic properties (OpenAPITools#6052)
[docs] Update README badges (OpenAPITools#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue OpenAPITools#7258 fix (OpenAPITools#7259)
[Dart] Get all enum values in a list (OpenAPITools#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (OpenAPITools#7273)
[core][bug] FILES is now path relative with no prefixes (OpenAPITools#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (OpenAPITools#7268)
[Java][jersey2] Add jersey injection dependencies (OpenAPITools#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (OpenAPITools#7255)
Date format dart (OpenAPITools#6389)
minor enhancement to java client generator (OpenAPITools#7253)
typescript: Fix Union Types Import Issue (OpenAPITools#6789)
Modifying the es5 and es6 templates for javascript to handle default values (OpenAPITools#6649)
[python-exp] simplify examples (OpenAPITools#7157)
Support for KumuluzEE microprofile runtime (OpenAPITools#5944)
[C#][netcore] minor improvements and bug fixes (OpenAPITools#7244)
...
@wing328wing328 changed the title [Javascript] Fix for constructors not handling required fields with default values well #6648[Javascript] Fix for constructors not handling required fields with default values wellSep 2, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tray2100@wing328@ybelenko
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[Javascript] Fix for constructors not handling required fields with default values well - #6649

Merged
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default
Aug 20, 2020
Merged

[Javascript] Fix for constructors not handling required fields with default values well#6649
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default

Conversation

@tray2100

Copy link
Copy Markdown
Contributor

This PR is for issue #6648. I verified that the output was as expected where for a spec that contains a required field with a default value will now look like

_this['field']=field||'defaultValue';

instead of just

_this['field']=field;

where it forces the required field to be undefined even though we have a default value. I also noticed that the samples were out of date and there wasn't a generate script for ES5 so I've added those here. I can squash the commits if needed before merging.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc: @CodeNinjai@frol@cliffano

@wing328

Copy link
Copy Markdown
Member

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

@tray2100

Copy link
Copy Markdown
ContributorAuthor

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

Hey @wing328

I probably should have shared more of my use case. I've defined a oneOf entity and made the discriminator required in the schemas referenced in it. It feels super awkward right now when using the constructFromObject mechanism because you'll have to manually add in the discriminator after the object is constructed - even if you used the entity created by the generator in the first place. It's even more awkward if there is a chain of constructFromObject calls and the entity with the discriminator is a few levels down the object graph.

Setting the field to the default value if nothing is specified solves this problem in a minimalist way.

This is essentially the spec:

 OneOfDogCat:
oneOf:
- $ref: "#/components/schemas/Dog"
- $ref: "#/components/schemas/Cat"
type: "object"
discriminator:
propertyName: "entityType"
Dog:
type: "object"
properties:
entityType:
default: "dog"
type: "string"
name:
type: "string"
required:
- "entityType"
Cat:
type: "object"
properties:
entityType:
default: "cat"
type: "string"
name:
type: "string"
required:
- "entityType"

…values
for required fields better. This change ensures the required field isn't
overridden with undefined when the object is constructed - especially through
a chain of constructFromObject calls..
@wing328

Copy link
Copy Markdown
Member

@tray2100 as discussed we can use {{#oneOf}} .. {{/oneOf}} to further enhance this fix for oneOf models only. Let's see if the community can help out with this.

@wing328
wing328 merged commit 71321bd into OpenAPITools:masterAug 20, 2020
@wing328wing328 added this to the 5.0.0 milestone Aug 20, 2020
jimschubert added a commit that referenced this pull request Aug 23, 2020
* master: (720 commits)
[docs] Update README badges (#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue #7258 fix (#7259)
[Dart] Get all enum values in a list (#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (#7273)
[core][bug] FILES is now path relative with no prefixes (#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (#7268)
[Java][jersey2] Add jersey injection dependencies (#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (#7255)
Date format dart (#6389)
minor enhancement to java client generator (#7253)
typescript: Fix Union Types Import Issue (#6789)
Modifying the es5 and es6 templates for javascript to handle default values (#6649)
[python-exp] simplify examples (#7157)
Support for KumuluzEE microprofile runtime (#5944)
[C#][netcore] minor improvements and bug fixes (#7244)
Deprecate Flash (ActionScript) client generator (#7231)
...
jimschubert added a commit to mohamedelhabib/openapi-generator that referenced this pull request Aug 24, 2020
* master: (219 commits)
[java] Appropriate instantiation of model with dynamic properties (OpenAPITools#6052)
[docs] Update README badges (OpenAPITools#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue OpenAPITools#7258 fix (OpenAPITools#7259)
[Dart] Get all enum values in a list (OpenAPITools#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (OpenAPITools#7273)
[core][bug] FILES is now path relative with no prefixes (OpenAPITools#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (OpenAPITools#7268)
[Java][jersey2] Add jersey injection dependencies (OpenAPITools#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (OpenAPITools#7255)
Date format dart (OpenAPITools#6389)
minor enhancement to java client generator (OpenAPITools#7253)
typescript: Fix Union Types Import Issue (OpenAPITools#6789)
Modifying the es5 and es6 templates for javascript to handle default values (OpenAPITools#6649)
[python-exp] simplify examples (OpenAPITools#7157)
Support for KumuluzEE microprofile runtime (OpenAPITools#5944)
[C#][netcore] minor improvements and bug fixes (OpenAPITools#7244)
...
@wing328wing328 changed the title [Javascript] Fix for constructors not handling required fields with default values well #6648[Javascript] Fix for constructors not handling required fields with default values wellSep 2, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tray2100@wing328@ybelenko
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[Javascript] Fix for constructors not handling required fields with default values well - #6649

Merged
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default
Aug 20, 2020
Merged

[Javascript] Fix for constructors not handling required fields with default values well#6649
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default

Conversation

@tray2100

Copy link
Copy Markdown
Contributor

This PR is for issue #6648. I verified that the output was as expected where for a spec that contains a required field with a default value will now look like

_this['field']=field||'defaultValue';

instead of just

_this['field']=field;

where it forces the required field to be undefined even though we have a default value. I also noticed that the samples were out of date and there wasn't a generate script for ES5 so I've added those here. I can squash the commits if needed before merging.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc: @CodeNinjai@frol@cliffano

@wing328

Copy link
Copy Markdown
Member

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

@tray2100

Copy link
Copy Markdown
ContributorAuthor

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

Hey @wing328

I probably should have shared more of my use case. I've defined a oneOf entity and made the discriminator required in the schemas referenced in it. It feels super awkward right now when using the constructFromObject mechanism because you'll have to manually add in the discriminator after the object is constructed - even if you used the entity created by the generator in the first place. It's even more awkward if there is a chain of constructFromObject calls and the entity with the discriminator is a few levels down the object graph.

Setting the field to the default value if nothing is specified solves this problem in a minimalist way.

This is essentially the spec:

 OneOfDogCat:
oneOf:
- $ref: "#/components/schemas/Dog"
- $ref: "#/components/schemas/Cat"
type: "object"
discriminator:
propertyName: "entityType"
Dog:
type: "object"
properties:
entityType:
default: "dog"
type: "string"
name:
type: "string"
required:
- "entityType"
Cat:
type: "object"
properties:
entityType:
default: "cat"
type: "string"
name:
type: "string"
required:
- "entityType"

…values
for required fields better. This change ensures the required field isn't
overridden with undefined when the object is constructed - especially through
a chain of constructFromObject calls..
@wing328

Copy link
Copy Markdown
Member

@tray2100 as discussed we can use {{#oneOf}} .. {{/oneOf}} to further enhance this fix for oneOf models only. Let's see if the community can help out with this.

@wing328
wing328 merged commit 71321bd into OpenAPITools:masterAug 20, 2020
@wing328wing328 added this to the 5.0.0 milestone Aug 20, 2020
jimschubert added a commit that referenced this pull request Aug 23, 2020
* master: (720 commits)
[docs] Update README badges (#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue #7258 fix (#7259)
[Dart] Get all enum values in a list (#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (#7273)
[core][bug] FILES is now path relative with no prefixes (#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (#7268)
[Java][jersey2] Add jersey injection dependencies (#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (#7255)
Date format dart (#6389)
minor enhancement to java client generator (#7253)
typescript: Fix Union Types Import Issue (#6789)
Modifying the es5 and es6 templates for javascript to handle default values (#6649)
[python-exp] simplify examples (#7157)
Support for KumuluzEE microprofile runtime (#5944)
[C#][netcore] minor improvements and bug fixes (#7244)
Deprecate Flash (ActionScript) client generator (#7231)
...
jimschubert added a commit to mohamedelhabib/openapi-generator that referenced this pull request Aug 24, 2020
* master: (219 commits)
[java] Appropriate instantiation of model with dynamic properties (OpenAPITools#6052)
[docs] Update README badges (OpenAPITools#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue OpenAPITools#7258 fix (OpenAPITools#7259)
[Dart] Get all enum values in a list (OpenAPITools#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (OpenAPITools#7273)
[core][bug] FILES is now path relative with no prefixes (OpenAPITools#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (OpenAPITools#7268)
[Java][jersey2] Add jersey injection dependencies (OpenAPITools#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (OpenAPITools#7255)
Date format dart (OpenAPITools#6389)
minor enhancement to java client generator (OpenAPITools#7253)
typescript: Fix Union Types Import Issue (OpenAPITools#6789)
Modifying the es5 and es6 templates for javascript to handle default values (OpenAPITools#6649)
[python-exp] simplify examples (OpenAPITools#7157)
Support for KumuluzEE microprofile runtime (OpenAPITools#5944)
[C#][netcore] minor improvements and bug fixes (OpenAPITools#7244)
...
@wing328wing328 changed the title [Javascript] Fix for constructors not handling required fields with default values well #6648[Javascript] Fix for constructors not handling required fields with default values wellSep 2, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tray2100@wing328@ybelenko
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[Javascript] Fix for constructors not handling required fields with default values well - #6649

Merged
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default
Aug 20, 2020
Merged

[Javascript] Fix for constructors not handling required fields with default values well#6649
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default

Conversation

@tray2100

Copy link
Copy Markdown
Contributor

This PR is for issue #6648. I verified that the output was as expected where for a spec that contains a required field with a default value will now look like

_this['field']=field||'defaultValue';

instead of just

_this['field']=field;

where it forces the required field to be undefined even though we have a default value. I also noticed that the samples were out of date and there wasn't a generate script for ES5 so I've added those here. I can squash the commits if needed before merging.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc: @CodeNinjai@frol@cliffano

@wing328

Copy link
Copy Markdown
Member

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

@tray2100

Copy link
Copy Markdown
ContributorAuthor

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

Hey @wing328

I probably should have shared more of my use case. I've defined a oneOf entity and made the discriminator required in the schemas referenced in it. It feels super awkward right now when using the constructFromObject mechanism because you'll have to manually add in the discriminator after the object is constructed - even if you used the entity created by the generator in the first place. It's even more awkward if there is a chain of constructFromObject calls and the entity with the discriminator is a few levels down the object graph.

Setting the field to the default value if nothing is specified solves this problem in a minimalist way.

This is essentially the spec:

 OneOfDogCat:
oneOf:
- $ref: "#/components/schemas/Dog"
- $ref: "#/components/schemas/Cat"
type: "object"
discriminator:
propertyName: "entityType"
Dog:
type: "object"
properties:
entityType:
default: "dog"
type: "string"
name:
type: "string"
required:
- "entityType"
Cat:
type: "object"
properties:
entityType:
default: "cat"
type: "string"
name:
type: "string"
required:
- "entityType"

…values
for required fields better. This change ensures the required field isn't
overridden with undefined when the object is constructed - especially through
a chain of constructFromObject calls..
@wing328

Copy link
Copy Markdown
Member

@tray2100 as discussed we can use {{#oneOf}} .. {{/oneOf}} to further enhance this fix for oneOf models only. Let's see if the community can help out with this.

@wing328
wing328 merged commit 71321bd into OpenAPITools:masterAug 20, 2020
@wing328wing328 added this to the 5.0.0 milestone Aug 20, 2020
jimschubert added a commit that referenced this pull request Aug 23, 2020
* master: (720 commits)
[docs] Update README badges (#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue #7258 fix (#7259)
[Dart] Get all enum values in a list (#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (#7273)
[core][bug] FILES is now path relative with no prefixes (#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (#7268)
[Java][jersey2] Add jersey injection dependencies (#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (#7255)
Date format dart (#6389)
minor enhancement to java client generator (#7253)
typescript: Fix Union Types Import Issue (#6789)
Modifying the es5 and es6 templates for javascript to handle default values (#6649)
[python-exp] simplify examples (#7157)
Support for KumuluzEE microprofile runtime (#5944)
[C#][netcore] minor improvements and bug fixes (#7244)
Deprecate Flash (ActionScript) client generator (#7231)
...
jimschubert added a commit to mohamedelhabib/openapi-generator that referenced this pull request Aug 24, 2020
* master: (219 commits)
[java] Appropriate instantiation of model with dynamic properties (OpenAPITools#6052)
[docs] Update README badges (OpenAPITools#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue OpenAPITools#7258 fix (OpenAPITools#7259)
[Dart] Get all enum values in a list (OpenAPITools#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (OpenAPITools#7273)
[core][bug] FILES is now path relative with no prefixes (OpenAPITools#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (OpenAPITools#7268)
[Java][jersey2] Add jersey injection dependencies (OpenAPITools#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (OpenAPITools#7255)
Date format dart (OpenAPITools#6389)
minor enhancement to java client generator (OpenAPITools#7253)
typescript: Fix Union Types Import Issue (OpenAPITools#6789)
Modifying the es5 and es6 templates for javascript to handle default values (OpenAPITools#6649)
[python-exp] simplify examples (OpenAPITools#7157)
Support for KumuluzEE microprofile runtime (OpenAPITools#5944)
[C#][netcore] minor improvements and bug fixes (OpenAPITools#7244)
...
@wing328wing328 changed the title [Javascript] Fix for constructors not handling required fields with default values well #6648[Javascript] Fix for constructors not handling required fields with default values wellSep 2, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tray2100@wing328@ybelenko
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[Javascript] Fix for constructors not handling required fields with default values well - #6649

Merged
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default
Aug 20, 2020
Merged

[Javascript] Fix for constructors not handling required fields with default values well#6649
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default

Conversation

@tray2100

Copy link
Copy Markdown
Contributor

This PR is for issue #6648. I verified that the output was as expected where for a spec that contains a required field with a default value will now look like

_this['field']=field||'defaultValue';

instead of just

_this['field']=field;

where it forces the required field to be undefined even though we have a default value. I also noticed that the samples were out of date and there wasn't a generate script for ES5 so I've added those here. I can squash the commits if needed before merging.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc: @CodeNinjai@frol@cliffano

@wing328

Copy link
Copy Markdown
Member

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

@tray2100

Copy link
Copy Markdown
ContributorAuthor

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

Hey @wing328

I probably should have shared more of my use case. I've defined a oneOf entity and made the discriminator required in the schemas referenced in it. It feels super awkward right now when using the constructFromObject mechanism because you'll have to manually add in the discriminator after the object is constructed - even if you used the entity created by the generator in the first place. It's even more awkward if there is a chain of constructFromObject calls and the entity with the discriminator is a few levels down the object graph.

Setting the field to the default value if nothing is specified solves this problem in a minimalist way.

This is essentially the spec:

 OneOfDogCat:
oneOf:
- $ref: "#/components/schemas/Dog"
- $ref: "#/components/schemas/Cat"
type: "object"
discriminator:
propertyName: "entityType"
Dog:
type: "object"
properties:
entityType:
default: "dog"
type: "string"
name:
type: "string"
required:
- "entityType"
Cat:
type: "object"
properties:
entityType:
default: "cat"
type: "string"
name:
type: "string"
required:
- "entityType"

…values
for required fields better. This change ensures the required field isn't
overridden with undefined when the object is constructed - especially through
a chain of constructFromObject calls..
@wing328

Copy link
Copy Markdown
Member

@tray2100 as discussed we can use {{#oneOf}} .. {{/oneOf}} to further enhance this fix for oneOf models only. Let's see if the community can help out with this.

@wing328
wing328 merged commit 71321bd into OpenAPITools:masterAug 20, 2020
@wing328wing328 added this to the 5.0.0 milestone Aug 20, 2020
jimschubert added a commit that referenced this pull request Aug 23, 2020
* master: (720 commits)
[docs] Update README badges (#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue #7258 fix (#7259)
[Dart] Get all enum values in a list (#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (#7273)
[core][bug] FILES is now path relative with no prefixes (#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (#7268)
[Java][jersey2] Add jersey injection dependencies (#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (#7255)
Date format dart (#6389)
minor enhancement to java client generator (#7253)
typescript: Fix Union Types Import Issue (#6789)
Modifying the es5 and es6 templates for javascript to handle default values (#6649)
[python-exp] simplify examples (#7157)
Support for KumuluzEE microprofile runtime (#5944)
[C#][netcore] minor improvements and bug fixes (#7244)
Deprecate Flash (ActionScript) client generator (#7231)
...
jimschubert added a commit to mohamedelhabib/openapi-generator that referenced this pull request Aug 24, 2020
* master: (219 commits)
[java] Appropriate instantiation of model with dynamic properties (OpenAPITools#6052)
[docs] Update README badges (OpenAPITools#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue OpenAPITools#7258 fix (OpenAPITools#7259)
[Dart] Get all enum values in a list (OpenAPITools#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (OpenAPITools#7273)
[core][bug] FILES is now path relative with no prefixes (OpenAPITools#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (OpenAPITools#7268)
[Java][jersey2] Add jersey injection dependencies (OpenAPITools#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (OpenAPITools#7255)
Date format dart (OpenAPITools#6389)
minor enhancement to java client generator (OpenAPITools#7253)
typescript: Fix Union Types Import Issue (OpenAPITools#6789)
Modifying the es5 and es6 templates for javascript to handle default values (OpenAPITools#6649)
[python-exp] simplify examples (OpenAPITools#7157)
Support for KumuluzEE microprofile runtime (OpenAPITools#5944)
[C#][netcore] minor improvements and bug fixes (OpenAPITools#7244)
...
@wing328wing328 changed the title [Javascript] Fix for constructors not handling required fields with default values well #6648[Javascript] Fix for constructors not handling required fields with default values wellSep 2, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tray2100@wing328@ybelenko
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[Javascript] Fix for constructors not handling required fields with default values well - #6649

Merged
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default
Aug 20, 2020
Merged

[Javascript] Fix for constructors not handling required fields with default values well#6649
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default

Conversation

@tray2100

Copy link
Copy Markdown
Contributor

This PR is for issue #6648. I verified that the output was as expected where for a spec that contains a required field with a default value will now look like

_this['field']=field||'defaultValue';

instead of just

_this['field']=field;

where it forces the required field to be undefined even though we have a default value. I also noticed that the samples were out of date and there wasn't a generate script for ES5 so I've added those here. I can squash the commits if needed before merging.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc: @CodeNinjai@frol@cliffano

@wing328

Copy link
Copy Markdown
Member

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

@tray2100

Copy link
Copy Markdown
ContributorAuthor

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

Hey @wing328

I probably should have shared more of my use case. I've defined a oneOf entity and made the discriminator required in the schemas referenced in it. It feels super awkward right now when using the constructFromObject mechanism because you'll have to manually add in the discriminator after the object is constructed - even if you used the entity created by the generator in the first place. It's even more awkward if there is a chain of constructFromObject calls and the entity with the discriminator is a few levels down the object graph.

Setting the field to the default value if nothing is specified solves this problem in a minimalist way.

This is essentially the spec:

 OneOfDogCat:
oneOf:
- $ref: "#/components/schemas/Dog"
- $ref: "#/components/schemas/Cat"
type: "object"
discriminator:
propertyName: "entityType"
Dog:
type: "object"
properties:
entityType:
default: "dog"
type: "string"
name:
type: "string"
required:
- "entityType"
Cat:
type: "object"
properties:
entityType:
default: "cat"
type: "string"
name:
type: "string"
required:
- "entityType"

…values
for required fields better. This change ensures the required field isn't
overridden with undefined when the object is constructed - especially through
a chain of constructFromObject calls..
@wing328

Copy link
Copy Markdown
Member

@tray2100 as discussed we can use {{#oneOf}} .. {{/oneOf}} to further enhance this fix for oneOf models only. Let's see if the community can help out with this.

@wing328
wing328 merged commit 71321bd into OpenAPITools:masterAug 20, 2020
@wing328wing328 added this to the 5.0.0 milestone Aug 20, 2020
jimschubert added a commit that referenced this pull request Aug 23, 2020
* master: (720 commits)
[docs] Update README badges (#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue #7258 fix (#7259)
[Dart] Get all enum values in a list (#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (#7273)
[core][bug] FILES is now path relative with no prefixes (#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (#7268)
[Java][jersey2] Add jersey injection dependencies (#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (#7255)
Date format dart (#6389)
minor enhancement to java client generator (#7253)
typescript: Fix Union Types Import Issue (#6789)
Modifying the es5 and es6 templates for javascript to handle default values (#6649)
[python-exp] simplify examples (#7157)
Support for KumuluzEE microprofile runtime (#5944)
[C#][netcore] minor improvements and bug fixes (#7244)
Deprecate Flash (ActionScript) client generator (#7231)
...
jimschubert added a commit to mohamedelhabib/openapi-generator that referenced this pull request Aug 24, 2020
* master: (219 commits)
[java] Appropriate instantiation of model with dynamic properties (OpenAPITools#6052)
[docs] Update README badges (OpenAPITools#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue OpenAPITools#7258 fix (OpenAPITools#7259)
[Dart] Get all enum values in a list (OpenAPITools#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (OpenAPITools#7273)
[core][bug] FILES is now path relative with no prefixes (OpenAPITools#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (OpenAPITools#7268)
[Java][jersey2] Add jersey injection dependencies (OpenAPITools#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (OpenAPITools#7255)
Date format dart (OpenAPITools#6389)
minor enhancement to java client generator (OpenAPITools#7253)
typescript: Fix Union Types Import Issue (OpenAPITools#6789)
Modifying the es5 and es6 templates for javascript to handle default values (OpenAPITools#6649)
[python-exp] simplify examples (OpenAPITools#7157)
Support for KumuluzEE microprofile runtime (OpenAPITools#5944)
[C#][netcore] minor improvements and bug fixes (OpenAPITools#7244)
...
@wing328wing328 changed the title [Javascript] Fix for constructors not handling required fields with default values well #6648[Javascript] Fix for constructors not handling required fields with default values wellSep 2, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tray2100@wing328@ybelenko
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[Javascript] Fix for constructors not handling required fields with default values well - #6649

Merged
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default
Aug 20, 2020
Merged

[Javascript] Fix for constructors not handling required fields with default values well#6649
wing328 merged 1 commit into
OpenAPITools:masterfrom
tray2100:javascript-default

Conversation

@tray2100

Copy link
Copy Markdown
Contributor

This PR is for issue #6648. I verified that the output was as expected where for a spec that contains a required field with a default value will now look like

_this['field']=field||'defaultValue';

instead of just

_this['field']=field;

where it forces the required field to be undefined even though we have a default value. I also noticed that the samples were out of date and there wasn't a generate script for ES5 so I've added those here. I can squash the commits if needed before merging.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc: @CodeNinjai@frol@cliffano

@wing328

Copy link
Copy Markdown
Member

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

@tray2100

Copy link
Copy Markdown
ContributorAuthor

@tray2100 thanks for the PR. If a field is required, there shouldn't be any default value since the value must be provided.

In other words, the default value is for the optional field, right?

Hey @wing328

I probably should have shared more of my use case. I've defined a oneOf entity and made the discriminator required in the schemas referenced in it. It feels super awkward right now when using the constructFromObject mechanism because you'll have to manually add in the discriminator after the object is constructed - even if you used the entity created by the generator in the first place. It's even more awkward if there is a chain of constructFromObject calls and the entity with the discriminator is a few levels down the object graph.

Setting the field to the default value if nothing is specified solves this problem in a minimalist way.

This is essentially the spec:

 OneOfDogCat:
oneOf:
- $ref: "#/components/schemas/Dog"
- $ref: "#/components/schemas/Cat"
type: "object"
discriminator:
propertyName: "entityType"
Dog:
type: "object"
properties:
entityType:
default: "dog"
type: "string"
name:
type: "string"
required:
- "entityType"
Cat:
type: "object"
properties:
entityType:
default: "cat"
type: "string"
name:
type: "string"
required:
- "entityType"

…values
for required fields better. This change ensures the required field isn't
overridden with undefined when the object is constructed - especially through
a chain of constructFromObject calls..
@wing328

Copy link
Copy Markdown
Member

@tray2100 as discussed we can use {{#oneOf}} .. {{/oneOf}} to further enhance this fix for oneOf models only. Let's see if the community can help out with this.

@wing328
wing328 merged commit 71321bd into OpenAPITools:masterAug 20, 2020
@wing328wing328 added this to the 5.0.0 milestone Aug 20, 2020
jimschubert added a commit that referenced this pull request Aug 23, 2020
* master: (720 commits)
[docs] Update README badges (#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue #7258 fix (#7259)
[Dart] Get all enum values in a list (#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (#7273)
[core][bug] FILES is now path relative with no prefixes (#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (#7268)
[Java][jersey2] Add jersey injection dependencies (#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (#7255)
Date format dart (#6389)
minor enhancement to java client generator (#7253)
typescript: Fix Union Types Import Issue (#6789)
Modifying the es5 and es6 templates for javascript to handle default values (#6649)
[python-exp] simplify examples (#7157)
Support for KumuluzEE microprofile runtime (#5944)
[C#][netcore] minor improvements and bug fixes (#7244)
Deprecate Flash (ActionScript) client generator (#7231)
...
jimschubert added a commit to mohamedelhabib/openapi-generator that referenced this pull request Aug 24, 2020
* master: (219 commits)
[java] Appropriate instantiation of model with dynamic properties (OpenAPITools#6052)
[docs] Update README badges (OpenAPITools#7276)
Update apiInvoker.mustache and sample file for akka-scala client for issue OpenAPITools#7258 fix (OpenAPITools#7259)
[Dart] Get all enum values in a list (OpenAPITools#7166)
Update .gitattributes
[ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false
Update check-supported-versions.yaml
[ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (OpenAPITools#7273)
[core][bug] FILES is now path relative with no prefixes (OpenAPITools#7271)
Update check-supported-versions.yaml
Update check-supported-versions.yaml (OpenAPITools#7268)
[Java][jersey2] Add jersey injection dependencies (OpenAPITools#7240)
[C][Clang Static Analyzer] Remove the useless variable when assembling URL (OpenAPITools#7255)
Date format dart (OpenAPITools#6389)
minor enhancement to java client generator (OpenAPITools#7253)
typescript: Fix Union Types Import Issue (OpenAPITools#6789)
Modifying the es5 and es6 templates for javascript to handle default values (OpenAPITools#6649)
[python-exp] simplify examples (OpenAPITools#7157)
Support for KumuluzEE microprofile runtime (OpenAPITools#5944)
[C#][netcore] minor improvements and bug fixes (OpenAPITools#7244)
...
@wing328wing328 changed the title [Javascript] Fix for constructors not handling required fields with default values well #6648[Javascript] Fix for constructors not handling required fields with default values wellSep 2, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tray2100@wing328@ybelenko