Repository navigation
Conversation
conjure-python deserializes compiled IR directly and never runs the compiler's parse-time validation, so identifiers in a definition that was not produced by the compiler can reach a code-emitting sink unchanged — for example, an enum value is emitted verbatim as a class-body assignment. Re-run the compiler's identifier and format validators over the deserialized definition before generating, rejecting inputs that do not conform to Conjure's grammar. ConjureDefinitionValidator.validateAll alone is insufficient: it performs only cross-cutting checks and does not format-check per-type identifiers such as enum values, which the compiler validates per type while parsing.
✅ Successfully generated changelog entry!Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. 📋Changelog Preview🐛 Fixes
|
| private GeneratorInputValidator() {} | ||
|
|
||
| public static void validate(ConjureDefinition definition) { | ||
| DealiasingTypeVisitor dealiasingTypeVisitor = new DealiasingTypeVisitor(definition.getTypes().stream() |
There was a problem hiding this comment.
This method is tightly coupled with a bunch of internal classes in conjure-core; this validation logic should be moved into the palantir/conjure repo. There's nothing Python specific here; we're checking if it adheres to the conjure spec. I'm not sure if the best organization is to include this as part of ConjureDefinitionValidator.validateAll or to expose it as a separate method; I haven't looked at that codepath closely.
There was a problem hiding this comment.
Done — moved upstream. This validation now lives in palantir/conjure as a new ConjureIrValidator (palantir/conjure#2149), and GeneratorInputValidator is removed from conjure-python entirely.
On the organization question: I exposed it as a dedicated entry point rather than folding it into ConjureDefinitionValidator.validateAll, because validateAll is already invoked mid-parse by ConjureParserUtils (after the per-type validators have run), so folding the per-type checks in would double-validate during parse and could shift error ordering. ConjureIrValidator composes the existing per-type validators (name/format, field, package, HTTP-path, endpoint) + validateAll, and leaves the parse path untouched; coverage was diffed against the ConjureParserUtils call sites.
| } | ||
|
|
||
| public void write(ConjureDefinition conjureDefinition, PythonFileWriter writer) { | ||
| GeneratorInputValidator.validate(conjureDefinition); |
There was a problem hiding this comment.
There should be a corresponding check in the generator as well that ensures that the Python code we're emitting is intentional and valid (like that identifiers are valid Python identifiers) - for whatever edge cases motivated this PR. We shouldn't be relying on the conjure spec for safety in the generated code (though it is still good to check defensively).
There was a problem hiding this comment.
Done. The generator now validates that every IR-derived string emitted into a Python identifier position is a valid Python identifier, independent of the spec — via @Value.Check on the poet snippets (PythonField, EnumSnippet, the BeanSnippet/UnionSnippet/AliasSnippet/PythonService class names, and PythonEndpointDefinition method/param names), using a new PythonIdentifierSanitizer.checkValidIdentifier.
Generation now fails fast on a malformed identifier rather than emitting malformed source, regardless of the IR. This is the safety mechanism the PR relies on; spec-conformance is the separate upstream concern (#2149).
Enforce that every IR-derived name emitted into a Python identifier position (class, attribute, enum member, method and parameter names) is a valid Python identifier, checked via @Value.Check on the poet snippets; add PythonIdentifierSanitizer.checkValidIdentifier for the check. Remove GeneratorInputValidator and revert conjure-core to a test-only dependency: spec-conformance validation of the input IR moves upstream, so the generator now enforces only the property it owns — that the Python it emits is well formed.
jppal9000
changed the title
…fiers # Conflicts: # versions.lock
dtobin
left a comment
dtobin
left a comment
There was a problem hiding this comment.
It's fine to pick up ConjureIrValidator in this PR as well - should be a one-line change
|
|
||
| @Value.Check | ||
| default void check() { | ||
| PythonIdentifierSanitizer.checkValidIdentifier(className()); |
There was a problem hiding this comment.
It's not just the className identifier we should be checking, it's basically everything we're inserting into the generated code via String.format. The generation code shouldn't make any assumptions about what checking has been previously performed by the IR validation.
The previous commit gated bare identifier sinks: class, attribute,
method, and enum value names. Other IR-derived values reach the
generated source in two further shapes, and neither was checked.
First, values emitted inside Python string literals, where a quote,
backslash, or control character can close the literal. Some of those
sinks had no check at all; the rest were protected only incidentally,
because a sibling identifier check happened to throw first. Add
isSafeStringLiteral and checkSafeStringLiteral, and call them from each
snippet's @Value.Check so the value is validated when the snippet is
built, before any source is emitted:
- PythonEndpointDefinition: HTTP path and method
- PythonEndpointParam: path param name, header and query parameter ids
- PythonField: JSON identifier
- Bean, Union, Enum and Service snippets: definition name and package
- AllSnippet: __all__ entries, as defense in depth
Second, values emitted as bare Python code, which cannot be quoted
because they have to stay evaluable: an alias target ("Alias = <expr>"),
the type handed to ConjureFieldDefinition, and mypy annotations. An
alias target is the sharpest of these, because it is an assignment at
module scope and so runs on import. Where the target is a type the IR
also declares, that type's own className check rejects a hostile name
first, but a reference to an undeclared type in the same package
generates no snippet and, since the package matches, no import either,
so nothing inspected it.
Neither existing check fits that shape: checkValidIdentifier would
reject legitimate generics such as Dict[str, List[int]], and
checkSafeStringLiteral guards quote boundaries that are not present
here, while still permitting the parentheses a call needs. Add
checkSafeTypeExpression, restricting these values to what the type
visitors can compose -- identifiers, dots, commas, spaces and square
brackets -- which admits generics but no call, assignment or statement
break. checkSafeTypeAnnotation additionally allows the double quotes
MyPyTypeNameVisitor puts around a forward reference.
Import module specifiers are also emitted as bare code and remain
ungated; they are dotted paths rather than type expressions, so they
need a separate check.
Imports are the remaining place the generator emits IR-derived values as
bare code. PythonImport writes "import <spec>" and "from <spec> import
(", and each entry in the list is rendered as "<name>" or
"<name> as <alias>". For a cross-package reference the specifier is
derived from the IR package name, and both halves of the aliased import
from IR type names, so none of them had been inspected.
A module specifier is a dotted module path rather than a type
expression, so checkSafeTypeExpression is not the right check for it:
it would allow the commas and brackets of a generic while a specifier
needs to permit a leading dot for a relative import such as .._impl.
Add checkSafeModuleSpecifier, restricting the specifier to identifier
characters and dots. Ending the import statement and starting another
requires a space, semicolon or newline, none of which survive that.
It deliberately does not enforce the full module-path grammar, since a
malformed path is a broken import rather than injected code.
The named import needs no new check. Both halves are plain Python
identifiers, so validating them on NamedImport with
checkValidIdentifier is stricter than pattern matching the rendered
line, and requires no parsing of it.
Before this PR
The generator placed identifiers taken from the input IR (class names, object
and enum member names, method and parameter names) into generated Python
without verifying they are valid Python identifiers.
After this PR
Identifiers emitted into a Python identifier position are validated when the
output model is built, so generation fails fast on a name that is not a valid
Python identifier instead of emitting malformed source.
Per review feedback, spec-conformance validation of the input IR is moved
upstream to palantir/conjure#2149 (a reusable
ConjureIrValidator); this PRkeeps only the generator-side guarantee that the Python it emits is well formed.
Possible downsides?
Definitions that would previously have produced malformed Python are now
rejected. Well-formed Conjure IR is unaffected.