gemini-code-assist[bot] commented on code in PR #37588:
URL: https://github.com/apache/beam/pull/37588#discussion_r2817843455
##########
sdks/python/apache_beam/yaml/json_utils.py:
##########
@@ -314,24 +314,34 @@ def _validate_compatible(weak_schema, strong_schema):
return
if weak_schema['type'] != strong_schema['type']:
raise ValueError(
- 'Incompatible types: %r vs %r' %
- (weak_schema['type'] != strong_schema['type']))
+ f"Incompatible types: {weak_schema['type']} vs
{strong_schema['type']}")
if weak_schema['type'] == 'array':
_validate_compatible(weak_schema['items'], strong_schema['items'])
- elif weak_schema == 'object':
+ elif weak_schema['type'] == 'object':
Review Comment:
The original condition `weak_schema == 'object'` was incorrect as
`weak_schema` is a dictionary. This change correctly accesses the 'type' key of
the dictionary to check if it's an 'object'. This was a critical bug.
##########
sdks/python/apache_beam/yaml/json_utils.py:
##########
@@ -314,24 +314,34 @@ def _validate_compatible(weak_schema, strong_schema):
return
if weak_schema['type'] != strong_schema['type']:
raise ValueError(
- 'Incompatible types: %r vs %r' %
- (weak_schema['type'] != strong_schema['type']))
+ f"Incompatible types: {weak_schema['type']} vs
{strong_schema['type']}")
Review Comment:
The original code here had a bug where `weak_schema['type'] !=
strong_schema['type']` (a boolean) was being printed twice instead of the
actual type values. The f-string correctly interpolates the type values, fixing
this logical error.
##########
sdks/python/apache_beam/yaml/json_utils.py:
##########
@@ -314,24 +314,34 @@ def _validate_compatible(weak_schema, strong_schema):
return
if weak_schema['type'] != strong_schema['type']:
raise ValueError(
- 'Incompatible types: %r vs %r' %
- (weak_schema['type'] != strong_schema['type']))
+ f"Incompatible types: {weak_schema['type']} vs
{strong_schema['type']}")
if weak_schema['type'] == 'array':
_validate_compatible(weak_schema['items'], strong_schema['items'])
- elif weak_schema == 'object':
+ elif weak_schema['type'] == 'object':
+ # If the weak schema allows for arbitrary keys (is a map),
+ # the strong schema must also allow for arbitrary keys.
+ if weak_schema.get('additionalProperties'):
+ if not strong_schema.get('additionalProperties', True):
+ raise ValueError('Incompatible types: map vs object')
+ _validate_compatible(
+ weak_schema['additionalProperties'],
+ strong_schema['additionalProperties'])
for required in strong_schema.get('required', []):
if required not in weak_schema['properties']:
- raise ValueError('Missing or unkown property %r' % required)
- for name, spec in weak_schema.get('properties', {}):
+ raise ValueError(f"Missing or unknown property '{required}'")
+ for name, spec in weak_schema.get('properties', {}).items():
Review Comment:
The original code `for name, spec in weak_schema.get('properties', {})`
would iterate over keys only. Adding `.items()` ensures that both the `name`
(key) and `spec` (value) are correctly retrieved for each property, which is
essential for the subsequent logic. This was a critical bug.
##########
sdks/python/apache_beam/yaml/json_utils.py:
##########
@@ -314,24 +314,34 @@ def _validate_compatible(weak_schema, strong_schema):
return
if weak_schema['type'] != strong_schema['type']:
raise ValueError(
- 'Incompatible types: %r vs %r' %
- (weak_schema['type'] != strong_schema['type']))
+ f"Incompatible types: {weak_schema['type']} vs
{strong_schema['type']}")
if weak_schema['type'] == 'array':
_validate_compatible(weak_schema['items'], strong_schema['items'])
- elif weak_schema == 'object':
+ elif weak_schema['type'] == 'object':
+ # If the weak schema allows for arbitrary keys (is a map),
+ # the strong schema must also allow for arbitrary keys.
+ if weak_schema.get('additionalProperties'):
+ if not strong_schema.get('additionalProperties', True):
+ raise ValueError('Incompatible types: map vs object')
+ _validate_compatible(
+ weak_schema['additionalProperties'],
+ strong_schema['additionalProperties'])
for required in strong_schema.get('required', []):
if required not in weak_schema['properties']:
- raise ValueError('Missing or unkown property %r' % required)
- for name, spec in weak_schema.get('properties', {}):
+ raise ValueError(f"Missing or unknown property '{required}'")
+ for name, spec in weak_schema.get('properties', {}).items():
+
if name in strong_schema['properties']:
try:
_validate_compatible(spec, strong_schema['properties'][name])
except Exception as exn:
- raise ValueError('Incompatible schema for %r' % name) from exn
- elif not strong_schema.get('additionalProperties'):
+ raise ValueError(f"Incompatible schema for '{name}'") from exn
+ elif not strong_schema.get('additionalProperties', True):
Review Comment:
The `additionalProperties` field in JSON Schema defaults to `True` if not
explicitly specified. The original code implicitly treated a missing
`additionalProperties` as `False`. By adding `True` as the default argument to
`get()`, this now correctly reflects the JSON Schema specification, preventing
incorrect validation for schemas that allow extra properties by default. This
was a high-severity bug.
##########
sdks/python/apache_beam/yaml/json_utils.py:
##########
@@ -314,24 +314,34 @@ def _validate_compatible(weak_schema, strong_schema):
return
if weak_schema['type'] != strong_schema['type']:
raise ValueError(
- 'Incompatible types: %r vs %r' %
- (weak_schema['type'] != strong_schema['type']))
+ f"Incompatible types: {weak_schema['type']} vs
{strong_schema['type']}")
if weak_schema['type'] == 'array':
_validate_compatible(weak_schema['items'], strong_schema['items'])
- elif weak_schema == 'object':
+ elif weak_schema['type'] == 'object':
+ # If the weak schema allows for arbitrary keys (is a map),
+ # the strong schema must also allow for arbitrary keys.
+ if weak_schema.get('additionalProperties'):
+ if not strong_schema.get('additionalProperties', True):
+ raise ValueError('Incompatible types: map vs object')
+ _validate_compatible(
+ weak_schema['additionalProperties'],
+ strong_schema['additionalProperties'])
for required in strong_schema.get('required', []):
if required not in weak_schema['properties']:
- raise ValueError('Missing or unkown property %r' % required)
- for name, spec in weak_schema.get('properties', {}):
+ raise ValueError(f"Missing or unknown property '{required}'")
+ for name, spec in weak_schema.get('properties', {}).items():
+
if name in strong_schema['properties']:
try:
_validate_compatible(spec, strong_schema['properties'][name])
except Exception as exn:
- raise ValueError('Incompatible schema for %r' % name) from exn
- elif not strong_schema.get('additionalProperties'):
+ raise ValueError(f"Incompatible schema for '{name}'") from exn
+ elif not strong_schema.get('additionalProperties', True):
+ # The property is not explicitly in the strong schema, and the strong
+ # schema does not allow for extra properties.
raise ValueError(
- 'Prohibited property: {property}; '
- 'perhaps additionalProperties: False is missing?')
+ f"Prohibited property: '{name}'; "
+ "perhaps additionalProperties: False is missing?")
Review Comment:
The f-string now correctly interpolates the `name` variable into the error
message, providing more specific and helpful feedback when a prohibited
property is encountered. The original `{property}` placeholder would not have
worked as intended.
##########
sdks/python/apache_beam/yaml/json_utils.py:
##########
@@ -314,24 +314,34 @@ def _validate_compatible(weak_schema, strong_schema):
return
if weak_schema['type'] != strong_schema['type']:
raise ValueError(
- 'Incompatible types: %r vs %r' %
- (weak_schema['type'] != strong_schema['type']))
+ f"Incompatible types: {weak_schema['type']} vs
{strong_schema['type']}")
if weak_schema['type'] == 'array':
_validate_compatible(weak_schema['items'], strong_schema['items'])
- elif weak_schema == 'object':
+ elif weak_schema['type'] == 'object':
+ # If the weak schema allows for arbitrary keys (is a map),
+ # the strong schema must also allow for arbitrary keys.
+ if weak_schema.get('additionalProperties'):
+ if not strong_schema.get('additionalProperties', True):
+ raise ValueError('Incompatible types: map vs object')
+ _validate_compatible(
+ weak_schema['additionalProperties'],
+ strong_schema['additionalProperties'])
for required in strong_schema.get('required', []):
if required not in weak_schema['properties']:
- raise ValueError('Missing or unkown property %r' % required)
- for name, spec in weak_schema.get('properties', {}):
+ raise ValueError(f"Missing or unknown property '{required}'")
Review Comment:
Corrected the typo from 'unkown' to 'unknown'. This improves the clarity of
the error message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
