Skip to content

Commit

Permalink
fix!: python models not handling circular model dependencies (#1946)
Browse files Browse the repository at this point in the history
  • Loading branch information
jonaslagoni authored Apr 25, 2024
1 parent b87ddc8 commit da14682
Show file tree
Hide file tree
Showing 15 changed files with 126 additions and 132 deletions.
2 changes: 2 additions & 0 deletions docs/languages/Python.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

There are special use-cases that each language supports; this document pertains to **Python models**.

The generated Python models require at least v[3.7](https://docs.python.org/release/3.7.0/).

<!-- toc is generated with GitHub Actions do not remove toc markers -->

<!-- toc -->
Expand Down
8 changes: 7 additions & 1 deletion docs/migrations/version-3-to-4.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ Console.WriteLine(dateTime2);

## Python

Models are aiming to be >= v3.7 compliant.

### Pydantic now follows v2 instead of v1

Reference: https://docs.pydantic.dev/2.6/migration/
Expand All @@ -104,7 +106,7 @@ class Message(BaseModel):
identifier: str = Field(description='''The Identifier for the Message''')
```

In Modelina 3 this used is rendered as:
In Modelina v3 this used is rendered as:

```python
class Message(BaseModel):
Expand Down Expand Up @@ -218,6 +220,10 @@ class Address:
return self._street_name
```

### Import style deprecation

All models are from this point onwards imported using explicit styles `from . import ${model.name}` to allow for circular model dependencies to work. This means that the option `importsStyle` is deprecated and is no longer in use. It will be removed at some point in the future.

## Go

### File names
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@

exports[`Should be able to render python models and should generate \`implicit\` or \`explicit\` imports for models implementing referenced types: nested-model 1`] = `
Array [
"
"from __future__ import annotations
from typing import Any, Dict
class ObjProperty:
def __init__(self, input: Dict):
if 'number' in input:
Expand All @@ -30,8 +31,9 @@ class ObjProperty:

exports[`Should be able to render python models and should generate \`implicit\` or \`explicit\` imports for models implementing referenced types: nested-model 2`] = `
Array [
"
"from __future__ import annotations
from typing import Any, Dict
class ObjProperty:
def __init__(self, input: Dict):
if 'number' in input:
Expand All @@ -58,14 +60,15 @@ class ObjProperty:

exports[`Should be able to render python models and should generate \`implicit\` or \`explicit\` imports for models implementing referenced types: root-model-explicit-import 1`] = `
Array [
"from ObjProperty import ObjProperty
"from __future__ import annotations
from typing import Any, Dict
from . import ObjProperty
class Root:
def __init__(self, input: Dict):
if 'email' in input:
self._email: str = input['email']
if 'obj_property' in input:
self._obj_property: ObjProperty = ObjProperty(input['obj_property'])
self._obj_property: ObjProperty.ObjProperty = ObjProperty.ObjProperty(input['obj_property'])
@property
def email(self) -> str:
Expand All @@ -75,25 +78,26 @@ class Root:
self._email = email
@property
def obj_property(self) -> ObjProperty:
def obj_property(self) -> ObjProperty.ObjProperty:
return self._obj_property
@obj_property.setter
def obj_property(self, obj_property: ObjProperty):
def obj_property(self, obj_property: ObjProperty.ObjProperty):
self._obj_property = obj_property
",
]
`;

exports[`Should be able to render python models and should generate \`implicit\` or \`explicit\` imports for models implementing referenced types: root-model-implicit-import 1`] = `
Array [
"from ObjProperty import ObjProperty
"from __future__ import annotations
from typing import Any, Dict
from . import ObjProperty
class Root:
def __init__(self, input: Dict):
if 'email' in input:
self._email: str = input['email']
if 'obj_property' in input:
self._obj_property: ObjProperty = ObjProperty(input['obj_property'])
self._obj_property: ObjProperty.ObjProperty = ObjProperty.ObjProperty(input['obj_property'])
@property
def email(self) -> str:
Expand All @@ -103,10 +107,10 @@ class Root:
self._email = email
@property
def obj_property(self) -> ObjProperty:
def obj_property(self) -> ObjProperty.ObjProperty:
return self._obj_property
@obj_property.setter
def obj_property(self, obj_property: ObjProperty):
def obj_property(self, obj_property: ObjProperty.ObjProperty):
self._obj_property = obj_property
",
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Array [
optional_field: Optional[str] = Field(description='''this field is optional''', default=None, serialization_alias='optionalField')
required_field: str = Field(description='''this field is required''', serialization_alias='requiredField')
no_description: Optional[str] = Field(default=None, serialization_alias='noDescription')
options: Optional[Options] = Field(default=None, serialization_alias='options')
options: Optional[Options.Options] = Field(default=None, serialization_alias='options')
",
]
`;
Expand Down
2 changes: 1 addition & 1 deletion src/generators/python/PythonConstrainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export const PythonDefaultTypeMapping: PythonTypeMapping = {
return constrainedModel.name;
},
Reference({ constrainedModel }): string {
return constrainedModel.name;
return `${constrainedModel.name}.${constrainedModel.name}`;
},
Any({ dependencyManager }): string {
dependencyManager.addDependency('from typing import Any');
Expand Down
42 changes: 32 additions & 10 deletions src/generators/python/PythonDependencyManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,7 @@ export class PythonDependencyManager extends AbstractDependencyManager {
* Simple helper function to render a dependency based on the module system that the user defines.
*/
renderDependency(model: ConstrainedMetaModel): string {
const useExplicitImports = this.options.importsStyle === 'explicit';
return `from ${useExplicitImports ? '.' : ''}${model.name} import ${
model.name
}`;
return `from . import ${model.name}`;
}

/**
Expand All @@ -26,14 +23,24 @@ export class PythonDependencyManager extends AbstractDependencyManager {
* For example `from typing import Dict` and `from typing import Any` would form a single import `from typing import Dict, Any`
*/
renderDependencies(): string[] {
let dependenciesToRender = this.dependencies;
dependenciesToRender =
this.mergeIndividualDependencies(dependenciesToRender);
dependenciesToRender = this.moveFutureDependency(dependenciesToRender);
return dependenciesToRender;
}

/**
* Split up each dependency that matches `from x import y` and keep everything else as is.
*
* Merge all `y` together and make sure they are unique and render the dependency as `from x import y1, y2, y3`
*/
private mergeIndividualDependencies(
individualDependencies: string[]
): string[] {
const importMap: Record<string, string[]> = {};
const dependenciesToRender = [];
/**
* Split up each dependency that matches `from x import y` and keep everything else as is.
*
* Merge all `y` together and make sure they are unique and render the dependency as `from x import y1, y2, y3`
*/
for (const dependency of this.dependencies) {
for (const dependency of individualDependencies) {
const regex = /from ([A-Za-z0-9]+) import ([A-Za-z0-9,\s]+)/g;
const matches = regex.exec(dependency);

Expand All @@ -58,4 +65,19 @@ export class PythonDependencyManager extends AbstractDependencyManager {
}
return dependenciesToRender;
}

/**
* If you import `from __future__ import x` it always has to be rendered first
*/
private moveFutureDependency(individualDependencies: string[]): string[] {
const futureDependencyIndex = individualDependencies.findIndex((element) =>
element.includes('__future__')
);
if (futureDependencyIndex !== -1) {
individualDependencies.unshift(
individualDependencies.splice(futureDependencyIndex, 1)[0]
);
}
return individualDependencies;
}
}
9 changes: 6 additions & 3 deletions src/generators/python/PythonGenerator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ export interface PythonOptions
extends CommonGeneratorOptions<PythonPreset, PythonDependencyManager> {
typeMapping: TypeMapping<PythonOptions, PythonDependencyManager>;
constraints: Constraints<PythonOptions>;
/**
* @deprecated no longer in use - we had to switch to using explicit import style, always to support circular model dependencies.
*/
importsStyle: 'explicit' | 'implicit';
}
export type PythonConstantConstraint = ConstantConstraint<PythonOptions>;
Expand All @@ -62,7 +65,7 @@ export class PythonGenerator extends AbstractGenerator<
defaultPreset: PYTHON_DEFAULT_PRESET,
typeMapping: PythonDefaultTypeMapping,
constraints: PythonDefaultConstraints,
importsStyle: 'implicit',
importsStyle: 'explicit',
// Temporarily set
dependencyManager: () => {
return {} as PythonDependencyManager;
Expand Down Expand Up @@ -206,8 +209,8 @@ export class PythonGenerator extends AbstractGenerator<
.map((model) => {
return dependencyManagerToUse.renderDependency(model);
});
const outputContent = `${modelDependencies.join('\n')}
${outputModel.dependencies.join('\n')}
const outputContent = `${outputModel.dependencies.join('\n')}
${modelDependencies.join('\n')}
${outputModel.result}`;
return RenderOutput.toRenderOutput({
result: outputContent,
Expand Down
28 changes: 16 additions & 12 deletions src/generators/python/presets/Pydantic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,30 +21,34 @@ const PYTHON_PYDANTIC_CLASS_PRESET: ClassPresetType<PythonOptions> = {
`class ${model.name}(BaseModel):`
);
},
property(params) {
let type = params.property.property.type;
const propertyName = params.property.propertyName;
property({ property, model, renderer }) {
let type = property.property.type;
const propertyName = property.propertyName;

if (params.property.property instanceof ConstrainedUnionModel) {
const unionTypes = params.property.property.union.map(
if (property.property instanceof ConstrainedUnionModel) {
const unionTypes = property.property.union.map(
(unionModel) => unionModel.type
);
type = `Union[${unionTypes.join(', ')}]`;
}

if (!params.property.required) {
if (!property.required) {
type = `Optional[${type}]`;
}
type = renderer.renderPropertyType({
modelType: model.type,
propertyType: type
});

const description = params.property.property.originalInput['description']
? `description='''${params.property.property.originalInput['description']}'''`
const description = property.property.originalInput['description']
? `description='''${property.property.originalInput['description']}'''`
: undefined;
const defaultValue = params.property.required ? undefined : 'default=None';
const jsonAlias = `serialization_alias='${params.property.unconstrainedPropertyName}'`;
const defaultValue = property.required ? undefined : 'default=None';
const jsonAlias = `serialization_alias='${property.unconstrainedPropertyName}'`;
let exclude = undefined;
if (
params.property.property instanceof ConstrainedDictionaryModel &&
params.property.property.serializationType === 'unwrap'
property.property instanceof ConstrainedDictionaryModel &&
property.property.serializationType === 'unwrap'
) {
exclude = 'exclude=True';
}
Expand Down
Loading

0 comments on commit da14682

Please sign in to comment.