-
Notifications
You must be signed in to change notification settings - Fork 5
DSCS-863: add minLength to all required schema fields, where applicable #138
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
base: main
Are you sure you want to change the base?
Changes from all commits
fe77072
01c372c
30ce68e
a0663b3
45a132d
c8340ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,8 @@ | |
| "properties": { | ||
| "unitId": { | ||
| "type": "string", | ||
| "maxLength": 256 | ||
| "maxLength": 256, | ||
| "minLength": 1 | ||
| }, | ||
| "serviceRank": { | ||
| "type": "integer" | ||
|
|
@@ -13,15 +14,18 @@ | |
| }, | ||
| "name": { | ||
| "type": "string", | ||
| "maxLength": 256 | ||
| "maxLength": 256, | ||
| "minLength": 1 | ||
| }, | ||
| "sku": { | ||
| "type": "string", | ||
| "maxLength": 64 | ||
| "maxLength": 64, | ||
| "minLength": 1 | ||
| }, | ||
| "url": { | ||
| "type": "string", | ||
| "maxLength": 2083 | ||
| "maxLength": 2083, | ||
| "minLength": 1 | ||
| }, | ||
| "imageUrl": { | ||
| "type": [ | ||
|
|
@@ -54,7 +58,8 @@ | |
| "type": "object", | ||
| "properties": { | ||
| "code": { | ||
| "type": "string" | ||
| "type": "string", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have rarely seen finalAdjustments being set in price data. Doesn't look like it's marked as required, we can remove this |
||
| "minLength": 1 | ||
| }, | ||
| "amount": { | ||
| "type": "number" | ||
|
|
@@ -73,7 +78,8 @@ | |
| "type": "object", | ||
| "properties": { | ||
| "code": { | ||
| "type": "string" | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "amount": { | ||
| "type": "number" | ||
|
|
@@ -114,7 +120,8 @@ | |
| "type": "object", | ||
| "properties": { | ||
| "code": { | ||
| "type": "string" | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "amount": { | ||
| "type": "number" | ||
|
|
@@ -133,7 +140,8 @@ | |
| "type": "object", | ||
| "properties": { | ||
| "code": { | ||
| "type": "string" | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "amount": { | ||
| "type": "number" | ||
|
|
@@ -183,8 +191,7 @@ | |
| "vendor": "com.adobe.magento.entity", | ||
| "name": "recommended-item", | ||
| "format": "jsonschema", | ||
| "version": "1-0-4" | ||
| "version": "1-0-5" | ||
| }, | ||
| "$schema": "http://iglucentral.com/schemas/com.snowplowanalytics.self-desc/schema/jsonschema/1-0-0#" | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,4 +41,3 @@ | |
| }, | ||
| "$schema": "http://iglucentral.com/schemas/com.snowplowanalytics.self-desc/schema/jsonschema/1-0-0#" | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,8 @@ | |
| "description": "Schema for Search Input Entity", | ||
| "properties": { | ||
| "searchUnitId": { | ||
| "type": "string" | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "source": { | ||
| "type": [ | ||
|
|
@@ -11,15 +12,17 @@ | |
| ] | ||
| }, | ||
| "queryTypes": { | ||
| "type": "array" | ||
| "type": "array", | ||
| "minLength": 1 | ||
| }, | ||
| "searchRequestId": { | ||
| "type": "string", | ||
| "maxLength": 256, | ||
| "minLength": 1 | ||
| }, | ||
| "query": { | ||
| "type": "string" | ||
| "type": "string", | ||
| "minLength": 1 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this maps to |
||
| }, | ||
| "page": { | ||
| "type": "number" | ||
|
|
@@ -83,7 +86,7 @@ | |
| "vendor": "com.adobe.magento.entity", | ||
| "name": "search-input", | ||
| "format": "jsonschema", | ||
| "version": "1-0-11" | ||
| "version": "2-0-1" | ||
| }, | ||
| "$schema": "http://iglucentral.com/schemas/com.snowplowanalytics.self-desc/schema/jsonschema/1-0-0#" | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,19 +2,23 @@ | |
| "description": "Schema for Product Search Result Entity", | ||
| "properties": { | ||
| "name": { | ||
| "type": "string" | ||
| "type": "string", | ||
| "minLength": 1 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens here if the search returns no results? We might still need the events but with empty fields? |
||
| }, | ||
| "url": { | ||
| "type": "string" | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "rank": { | ||
| "type": "number" | ||
| }, | ||
| "sku": { | ||
| "type": "string" | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "imageUrl": { | ||
| "type": "string" | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "price": { | ||
| "type": "number" | ||
|
|
@@ -33,8 +37,7 @@ | |
| "vendor": "com.adobe.magento.entity", | ||
| "name": "search-result-product", | ||
| "format": "jsonschema", | ||
| "version": "1-0-2" | ||
| "version": "1-0-3" | ||
| }, | ||
| "$schema": "http://iglucentral.com/schemas/com.snowplowanalytics.self-desc/schema/jsonschema/1-0-0#" | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I am thinking about this, should we also add
minLengthto thenamefield since it is also required?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why "name" is a required field. I don't think we have any filters related to that in the code today. If we do, we can consider adding minLength otherwise I'd leave it as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we are unsure of the effects then I'll just keep the minLength added to
skuand we can revisitnameat a different point. I'm not sure we can be sure of all of the ways adding minLength tonamewill change things 😬