Skip to content

fix: ODCS to .proto, problem with array of objects#1012

Open
Schokuroff wants to merge 9 commits intodatacontract:mainfrom
Schokuroff:fix/proto-exporter
Open

fix: ODCS to .proto, problem with array of objects#1012
Schokuroff wants to merge 9 commits intodatacontract:mainfrom
Schokuroff:fix/proto-exporter

Conversation

@Schokuroff
Copy link

@Schokuroff Schokuroff commented Jan 12, 2026

  • Tests pass
  • ruff format
  • README.md updated (if relevant)
  •  CHANGELOG.md entry added

@Schokuroff
Copy link
Author

schema:
- name: FsaRegister
  logicalType: object
  properties:
  - name: fsa_room
    logicalType: array
    items:
      name: FsaRoom
      logicalType: object
      properties:
      - name: rooms_count
        logicalType: integer
      - name: category_type
        logicalType: string
  - name: simple_array_int
    logicalType: array
    items:
      logicalType: integer
  - name: simple_int
    logicalType: integer
  - name: simple_obj
    logicalType: object
    properties:
    - name: rooms_count
      logicalType: integer
    - name: category_type
      logicalType: string

to

syntax = "proto3";

package example;

message FsaRegister {
message FsaRoom {
    int32 rooms_count = 1;
    string category_type = 2;
  }

message SimpleObj {
    int32 rooms_count = 1;
    string category_type = 2;
  }

  repeated FsaRoom fsa_room = 1;
  repeated int32 simple_array_int = 2;
  int32 simple_int = 3;
  SimpleObj simple_obj = 4;
}

@Schokuroff
Copy link
Author

Sorry, I think I forgot to do the ruff and tests, I'll add them now.

@Schokuroff Schokuroff changed the title fix export array of objects fix: ODCS to .proto, problem with array of objects Jan 12, 2026
@Schokuroff
Copy link
Author

@jochenchrist
wow, looks like all checks have passed, can we merge it?

@Schokuroff
Copy link
Author

I have done ruff check localy, but failed with "uv run pytest", oracle idk...

uv run pytest
========================================================= test session starts =========================================================
platform darwin -- Python 3.11.8, pytest-9.0.2, pluggy-1.6.0
rootdir: /Users/nshokurov/Documents/datacontract/datacontract-cli
configfile: pyproject.toml
plugins: anyio-4.12.1, xdist-3.8.0, typeguard-4.4.4
collected 261 items / 5 errors

=============================================================== ERRORS ================================================================
_____________________________________________ ERROR collecting tests/test_test_oracle.py ______________________________________________
.venv/lib/python3.11/site-packages/urllib3/connectionpool.py:787: in urlopen

@Schokuroff
Copy link
Author

@jochenchrist sooo, lets merge?

@Schokuroff
Copy link
Author

@jochenchrist hello! lets merge it?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Protobuf exporter to better handle ODCS schemas—specifically arrays of objects—by generating nested message definitions and updating the expected .proto output accordingly.

Changes:

  • Extend the Protobuf export test fixture to include an array-of-objects field (users) and a non-required field (geo_description).
  • Refactor Protobuf type/message naming and nested message generation logic to support arrays of objects.
  • Add support for emitting optional fields for required: false properties.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
tests/test_export_protobuf.py Adds coverage for arrays of objects and required: falseoptional output in generated Protobuf.
datacontract/export/protobuf_exporter.py Refactors Protobuf generation to create nested messages (including for arrays of objects) and emit optional for non-required fields.
Comments suppressed due to low confidence (1)

datacontract/export/protobuf_exporter.py:22

  • _get_config_value is typed as returning Optional[str], but cp.value can be a non-string (e.g., dict for enumValues, as used by _get_enum_values). This type mismatch can hide bugs in type checking; consider widening the return type (e.g., Optional[Any]) or making it generic.
def _get_config_value(prop: SchemaProperty, key: str) -> Optional[str]:
    """Get a custom property value from customProperties."""
    if prop.customProperties is None:
        return None
    for cp in prop.customProperties:
        if cp.property == key:
            return cp.value
    return None

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +149 to +150
# Use as-is (might already be in UpperCamelCase)
return prop.items.name
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

For arrays of objects, _get_type_name returns prop.items.name without normalizing it. However, to_protobuf_message() will UpperCamel-case the message name, which can cause a type mismatch (e.g., items.name=user → nested message User {} but field type repeated user). Normalize items.name the same way as message declarations (e.g., via _snake_to_upper_camel) so field references always match generated type names.

Suggested change
# Use as-is (might already be in UpperCamelCase)
return prop.items.name
# Normalize items.name the same way as message declarations
return _snake_to_upper_camel(prop.items.name)

Copilot uses AI. Check for mistakes.
Copy link
Author

Choose a reason for hiding this comment

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

I am not sure, we need it

Comment on lines +349 to +352

# Add 'optional' for non-required fields that are not arrays
if (hasattr(prop, 'required') and prop.required is False and
not (prop.logicalType and prop.logicalType.lower() == "array")):
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

_get_field_declaration unconditionally prefixes optional for required: false non-array fields. In proto3, optional is only valid for scalar/enum fields; applying it to message/object-typed fields can produce an invalid .proto. Gate this so optional is added only for scalar/enum field declarations (and never for message types).

Suggested change
# Add 'optional' for non-required fields that are not arrays
if (hasattr(prop, 'required') and prop.required is False and
not (prop.logicalType and prop.logicalType.lower() == "array")):
logical_type = (prop.logicalType or "").lower()
is_array = logical_type == "array"
is_message_type = logical_type in ["object", "record", "struct"]
# Add 'optional' only for non-required, non-array, non-message fields (scalars/enums)
if hasattr(prop, "required") and prop.required is False and not is_array and not is_message_type:

Copilot uses AI. Check for mistakes.
Comment on lines +324 to +325
# field_type = _get_field_type(prop) # includes "repeated" if needed
field_decl = _get_field_declaration(prop) # Новая функция
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

There’s leftover commented-out code (# field_type = ...) and a non-English inline comment (# Новая функция) in the message generation loop. Please remove the dead/commented code and use a consistent language for comments to keep the exporter maintainable.

Suggested change
# field_type = _get_field_type(prop) # includes "repeated" if needed
field_decl = _get_field_declaration(prop) # Новая функция
field_decl = _get_field_declaration(prop) # Use helper to build field declaration (handles optional/repeated)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants