When the user tries to modify the view with an invalid xpath expression,
an XPathSyntaxError traceback will appear.
Steps to produce:
1. Install the Accounting module.
2. Settings > Technical > UI > Views > Open any view
3. Invalidate expr syntax and try to save, thus an error will be generated.
Error: XPathSyntaxError: Invalid expression
This commit handles XPathSyntaxError by raising ValidationError
instead of a traceback.
sentry-4377014622
closesodoo/odoo#139435
X-original-commit: 2af583d0b803b3334b871002e447d3211eb09bc2
Signed-off-by: Achraf Ben Azzouz (abz) <abz@odoo.com>
`if isinstance(node, SKIPPED_ELEMENT_TYPES):`
---------------------------------------------
It has been added in
https://github.com/odoo/odoo/commit/31b9bee00676e179c124f3d42539103305cf7423
It's hard to tell why it has been added. Though it seems not useful:
- Has we iter the nodes using the tag of the specification,
`arch.iter(spec.tag)`,
this is normally impossible to iter on a node type from that
SKIPPED_ELEMENT_TYPES node type list
(`etree._Comment, etree._ProcessingInstruction, etree.CommentBase, etree.PIBase, etree._Entity`),
as those node types do not have a tag (`spec.tag`).
In addition, in the place calling `locate_node`,
we already do this same check on the specification node:
https://github.com/odoo/odoo/blob/48109c15d319ae69a8fcdd4a6c3b19172a2de6d9/odoo/tools/template_inheritance.py#L144-L150
Version spec should match parent's root element's version
---------------------------------------------------------
It has also been added in the same revision:
https://github.com/odoo/odoo/commit/31b9bee00676e179c124f3d42539103305cf7423
This has been added in 7.0 to handle `version="7.0"` in xml views.
However, these `version="7.0"` have been removed in 8.0, in revision
odoo/odoo@faa09da325
After the above revision, there were still some leftovers,
but which have all been removed in 11.0 in revision
odoo/odoo@70942e4cfb
during an improvement of the RNG validator.
This is therefore enough evidence to say this code
checking for the `version` attribute in `locate_node`
is no longer useful since a quite long time now.
It can therefore be removed.
closesodoo/odoo#107340
Signed-off-by: Denis Ledoux (dle) <dle@odoo.com>
Since [1] which tried to fix the data-oe-xpath branding on nodes in some
cases, the branding actually became potentially incorrect on siblings of
a node which is replaced multiple times. E.g.
Parent view:
```xml
<hello>
<world class="a"></world>
<world class="b"></world>
<world class="c"></world>
</hello>
```
Child view 1:
```xml
<xpath position="//world[hasclass('a')]" position="replace">
<world class="new_a"></world>
</xpath>
```
Child view 2:
```xml
<xpath position="//world[hasclass('b')]" position="replace">
<world class="new_b"></world>
</xpath>
```
No problem, two distincts elements are replaced, the system understands
that the `data-oe-xpath` of the third world of the parent view should be
`/hello[1]/world[3]`.
But in this other case:
Parent view:
```xml
<hello>
<world class="a"></world>
<world class="b"></world>
<world class="c"></world>
</hello>
```
Child view:
```xml
<xpath position="//world[hasclass('a')]" position="replace">
<world class="new_a"></world>
</xpath>
```
Child view of the child view:
```xml
<xpath position="//world[hasclass('new_a')]" position="replace">
<world class="another_new_a"></world>
</xpath>
```
The `data-oe-xpath` of the third world of the parent view (in the
resulting view) was wrong: `/hello[1]/world[4]` -> because the system
saw two replacements + the unreplaced second `<world>`, so the index "4"
was computed.
Now the system will understand that the double replacement in fact acts
as a single replacement.
Note: this was also the same with "cross inheriting" (if the "new_a"
`<world>` of the child view was replaced by another child view of the
parent view).
At last, another 4th case was found and worth mentioning because it is
in fact the root cause of the problem. The problem is not actually the
double replacement as mentioned above but simply the replacement of a
root level element of a child view (which is what is basically done in
the last two mentioned cases). In that case, the root level nodes added
by the first child view have already their `data-oe-xpath` branding
computed before they are potentially replaced. Indicating the location
of the replacement in that case was thus only leading to bugs. E.g.
Parent view:
```xml
<hello>
<world class="a"></world>
<world class="b"></world>
</hello>
```
Child view:
```xml
<xpath expr="//world[hasclass('a')]" position="after">
<world class="x"></world>
<world class="y"></world>
</xpath>
```
Child view of the child view:
```xml
<xpath expr="//world[hasclass('x')]" position="replace"/>
```
Before this commit, before the branding is distributed, the result is:
```xml
<hello data-oe-model="ir.ui.view" data-oe-id="1439" data-oe-field="arch">
<world class="a"/>
<?apply-inheritance-specs-node-removal world?>
<world class="y" data-oe-id="1440" data-oe-xpath="/data/xpath/world[2]" data-oe-model="ir.ui.view" data-oe-field="arch"/>
<world class="b"/>
</hello>
```
=> Hence the `data-oe-xpath` of the last `<world>` was computed to
`/hello[1]/world[3]` instead of `/hello[1]/world[2]` after branding
distribution because the ProcessingInstruction marking the node
removal location should not have been added: it could only be useful
to following siblings which are not branded, which is not possible as
the branding added on the second `<world>` of the child view
(`/data/xpath/world[2]`) was computed before any removal.
Tests are added in this commit for the 3 last mentioned cases. As
explained, the last case is actually the same of the 2nd and 3rd ones
but it was decided to keep the 3 tests as it helps to understand the
problems better and, if the code evolves, it could become different
cases (= this is 3 cases which are currently technically equivalent but
these are different functionnal use cases). A test was written for the
first case then removed as it is basically a pure copy of other existing
tests written in [2] (trying to be improved by [1]).
[1]: https://github.com/odoo/odoo/commit/f67832a3ae0d9a3b5b53129132762e6bc1aed874
[2]: https://github.com/odoo/odoo/commit/c077ef05575d9677bce284195683f96c68386788closesodoo/odoo#92589
X-original-commit: d6e0b3d570a4b27f72852eb261660ad09de12eeb
Signed-off-by: Romain Derie (rde) <rde@odoo.com>
Signed-off-by: Quentin Smetz (qsm) <qsm@odoo.com>
When branding was added on items which follow an element that is removed
by an inheriting view, the branding was incorrect. Indeed, it supposed
the removed element does not exist in the original view. E.g.:
Parent view:
```
<hello>
<world></world>
<world></world>
<t t-esc="foo"/>
</hello>
```
Child view:
```
<data>
<xpath expr="/hello/world[1]" position="replace"/>
</data>
```
=> There are two <world/> in the original view, the first one is removed
by a child view. The data-oe-xpath set on the remaining <world/>
should still be /hello/world[2] and not /hello/world[1] to target the
right element in the parent view.
This of course induced edition problems where a saved area was not saved
inside the right element of the parent view or, more likely in normally
complex arch, just crashed on save. As an example, with 14.0 enterprise:
- Install website_appointment
- Go to a page with a calendar to schedule an appointment
- Enter edit mode, try to add something in the area *below* the calendar
- Save => crash
Commit [1] already fixed similar problems when an element was *replaced*
by something (especially, when replaced by an element with the same tag
name). This commit actually reviews what was done to fix both problems
(replacement and removal) at the same time. It also makes it so the xml
that `apply_inheritance_specs` produces has no "Element" part impacted
when used with "inherit_branding=True", which seems better... although
the notion of inheriting branding should probably be independant from
this function (maybe something to do in master).
[1]: https://github.com/odoo/odoo/commit/c077ef05575d9677bce284195683f96c68386788
opw-2811674
X-original-commit: 4ab569933464617444f6150793876ff4eba11690
Part-of: odoo/odoo#91991
First step to stream QWeb templates: removing the two post
processing operations applied on rendered templates. This
should slightly speed up the rendering of every page.
1/ Don't remove empty lines after rendering, but fix the root
cause of: view inheritancies and QWeb compilation that don't
add extra empty lines.
2/ handle page break in the two reports that uses it, rather
than processing every view produced.
closesodoo/odoo#82244
Related: odoo/enterprise#23328
Signed-off-by: Fabien Pinckaers <fp@odoo.com>
This commit adds a new attribute mode for position 'replace' to the xpath feature.
This mode can take 2 values:
- 'outer' (default mode if not provided) that will replace the sibling target
- 'inner' that will preserve the sibling target
If base arch is:
```html
<p>
<field name='x'>yyy</field>
</p>
```
`<field name='x' position="replace" (mode="outer")>zzz</field>`
```html
<p>
zzz
</p>
```
`<field name='x' position="replace" mode="inner">zzz</field>`
```html
<p>
<field name='x'>
zzz
</field>
</p>
```
Mode inner is useful for theme or render flat content, but not recommanded to
be used in page editable since we ignore the inherit-branding part until now.
task-2172208
closesodoo/odoo#48679
Signed-off-by: Jérémy Kersten (jke) <jke@openerp.com>
Co-authored-by: Okan SUMER (osu) <osu@odoo.com>
Co-authored-by: Jérémy Kersten <jke@odoo.com>
Add a big fat warning when the qweb compiler finds a `t-raw`.
`t-esc` should now be used everywhere, the use-case for `t-raw` should
be handled by converting the corresponding values to `Markup`
objects. Even though it's convenient, this constructor *should never
be made available in the qweb rendering context* (maybe that should be
checked for explicitely?).
Replace `werkzeug.escape` by `markupsafe.escape` in
`odoo.tools.html_escape`, this means the output of `html_escape` is
markup-safe.
Updated qweb to work correctly with escaping and `Markup`, amongst
other things QWeb bodies should be markup-safe internally (so that a
`t-set` value can be fed into a `t-esc`). See at the bottom for the
attributes handling as it's a bit complicated.
`to_text` needed updating: `markupsafe.Markup` is a subclass of `str`,
but `str` is not a passthrough for strings. So `Markup` instances
going through would be converted to normal `str`, losing their safety
flag. Since qweb internally uses `to_text` on pretty much
everything (in order to handle None / False), this would then cause
almost every `Markup` to get mistakenly double-escaped.
Also mark a bunch of APIs as markup-safe by default
* html_sanitize output.
* HTML fields content, sanitization is applied on intake (so stripped
by the trip through the database) and if the field is unsanitised
the injection is very much intentional, probably. Note: this
includes automatically decoding bytes as a number of default values
& computes yield bytes, which Markup will happily accept... by
repr-ing them which is useless. This is hard to notice without `-b`.
* Script-safe json, it's rather the point (though it uses a
non-standard escaping scheme).
* Note that `nl2br`, kinda: it should work correctly whether or not
the input is markup-safe, this means we should not need to escape
values fed to `nl2br`, but it doesn't hurt either.
Update some qweb field serialisations to mark their output as
markup-safe when necessary (e.g. monetary, barcode,
contact). Otherwise either using proper escaping internally or doing
nothing should do the trick.
Also update qweb to return markup-safe bytes: we want qweb to return
markup-safe contents as a common use-case is to render something with
one template, and inject its content in an other one (with Python code
inbetween, as `t-call` works a bit differently and does not go through
the external rendering interface).
However qweb returns `bytes` while `Markup` extends `str`. After a
quick experiment with changing qweb rendering to return `str` (rather
unmitigated failure I fear), it looks like the safest tack is to add a
somewhat similar bytes-based type, which decodes to a `Markup` but
keeps to bytes semantics.
For debugging and convenience reasons, MarkupSafeBytes does *not*
stringify and raises an error instead (`__repr__` works fine). This is
to avoid implicit stringifications which do the wrong thing (namely
create a string `"b'foo'"`).
Also add some configuration around BytesWarning (which still has to be
enabled at the interpreter level via `-b`, there's no way to enable it
programmatically smh), and monkeypatch `showwarning` to show warning
tracebacks, as it's common for warnings to be triggered in the bowels
of the application, and hard to relate to business logic without the
complete traceback.
`t-out`
=======
`t-esc` is a bit confusing for the new behaviour of "maybe escape
maybe not", so add a `t-out` alias with the same behaviour.
Unlike `t-raw`, `t-esc` is only soft-deprecated for now: there are
thousands of instances, so editing all the templates is not
great. Eventually we'll add a `ci/style` to prevent addition of new
ones, and eventually we might do a bulk-replace and hard-deprecate.
Attributes handling
===================
There are a few issues with respect to attributes. The first issue is
that markup-safe content is not necessarily attributes-safe
e.g. markup-safe content can contain unescaped `<` or double-quotes
while attributes can not. So we must forcefully escape the input, even
if it's supposedly markup-safe already.
This causes a problem for script-safe JSON: it's markup-safe but
really does its own thing. So instead of escaping it up-front and
wrapping it in Markup, make script-safe JSON its own type which
applies JSON-escaping *during the `__html__` call.
This way if a script-safe JSON object goes through `markupsafe.escape`
we'll apply script-safe escaping, otherwise it'll be treated as a
regular strings and eventually escaped the normal way.
A second issue was the processing of format-valued
attributes (`t-attf`): literal segments should always be markup-safe,
while non-literal may or may not be. This turns out to be an issue if
the non-literal segment *is* markup-safe: in that case when the
literal and non-literal segments get concatenated the literal segments
will get escaped, then attributes serialization will escape
them *again* leading to doubly-escaped content in attributes.
The most visible instance of this was the `snippet_options` template,
specifically:
<t t-set="so_content_addition_selector" t-translation="off">blockquote, ...</t>
<div id="so_content_addition"
t-att-data-selector="so_content_addition_selector"
t-attf-data-drop-near="p, h1, h2, h3, .row > div > img, #{so_content_addition_selector}"
data-drop-in=".content, nav"/>
Here `so_content_addition_selector` is a qweb body therefore
markup-safe, When concatenated with the literal part of
`t-atff-data-drop-near` it would cause the HTML-escaping of that
yielding a new Markup object. Normal attributes processing would then
strip the markup flag (using `str()`) and escape it again, leading to
doubly-escaped literals.
The original hack around was to unescape() `Markup` content before
stringifying it and escaping it again, in the attribute serialization
method (`_append_attributes`).
That's pretty disgusting, after some more consideration & testing it
looks like a much better and safer fix is to ensure the
expression (non-literal) segments of format strings always result in
`str`, never `Markup`, which is easy enough: just all `str()` on the
output of strexpr. We could also have concatenated all the bits using
`''.join` instead of repeated concatenation (`+`).
Also add a check on the type of the format string for safety, I think
it should always be a proper str and the bytes thing is only when
running in py2 (where lxml uses bytestrings as a space optimization
for ascii-only values) but it should not hurt too much to perform a
single typecheck assertion on the value... instead of performing one
per literal segment.
Note: we may need to implement unescape anyway, because it's still
possible to get double-escaping with the current scheme: given an
explicitly escape-ed `foo` and `t-att-foo="foo"`, `foo` will be
re-escaped.
fixup! [CHG] core, web: deprecate t-raw
Using a few regex like
\((_\(.*%s.*)(\) % )([\w\[\]][\w .\[\]\(\)'"]*)\)
($1, $3))
Old syntax is still compatible but starts the migration to the new
syntax that catches error.
Before create_multi, in case of invalid syntax used in an XPath, only
the problematic record was displayed. It was not ideal for long
definition but still usable.
Since the views are created using create_multi, the whole file content
is displayed in the error traceback, making it almost impossible to
locate on files with multiple records.
closesodoo/odoo#43590
X-original-commit: 8622469e1fdd4789cc45ed52093d0f329abca14e
Signed-off-by: Jérémy Kersten (jke) <jke@openerp.com>
Before this commit, when a template was inheriting from another
the template mentionned its t-inherit
This has been deemed overkill as the information is irrelevant to the caller
i.e. the caller just wants the template and doesn't care how they've been computed
After this commit, only t-name and attributes not related with inheritance
are disclosed
[FIX]: base: static inheritance propagates other attributes
When doing a inherit in primary mode, the original attributes on the root node of
the inheriting template were not propagated
After this commit they are
At the conception of this feature it has been intentionally thought that
the behavior for static templates should resemble
what is done for ir.ui.view
While keeping the general previous behavior (and this is important)
A little bit of context for ir.ui.view
They are defined as XML's, but end up as python objects
Their meta-data (id, name, inheritance specs...) are thus
present in their XML definition, but end up as part of python objects members
Hence, the final, business, usable arch is free of those meta-data
and is left only with business-relevant dom nodes
The static inheritance feature was backed with those ideas
but inherently encountered the issue that, for them,
the meta-data also end up in the business dom, since
they are at no point considered as plain objects.
Decision has been made, back then, to exclude the root node
that holds the metadata, to be at all targeted by any XPATH
This decision is now challenged as the specs of XPATH should be respected
This commit consequently introduces the root node as any other
It can be replaced, and will be targeted by
`expr="."` or `expr="//NODE_TAG"`
The few attributes that are necessary to define them are kept across
inheritance cycles though.
Have a parent template
Have a child, in extension inherit mode of the parent
The child has a xpath like
`<xpath expr="." position="replace" />`
Before this commit, there was a crash.
That was because the Comment
(which indicates which templates modified the parent)
was taken as the replacer node
instead of the actual content of the xpath
After this commit, there is no crash, and it works as expected
closesodoo/odoo#39453
X-original-commit: 02d790e1d21b78395e489f57bf30c82f728cbee6
Signed-off-by: Lucas Perais (lpe) <lpe@odoo.com>
QWeb templates that show up in the 'qweb' key of a module's manifest
now support server side inheritance and xpath evaluation
QWeb templates that show up in the xmlDependencies of a JS widget are not
impacted at all by theses changes, as they are served through the
Werkzeug sharedMiddleware
A similar syntax than ir.ui.view has been implemented in the QWeb templates
- each template must have a root node, whatever tag works
- the root node of a template must have a t-name containing the name of the template
The name -- without the module's name -- may contain dots pretty much anywhere
Though what is recommended is only underscores in template names
- if a template is to inherit from a parent, the root node has a t-inherit directive
containing either the full name of the template it inherits from which is module_name.template_name
or the name of the template, no module name necessary, if the parent template is in the same module
- there are 2 modes of inheriting
primary: copy the behavior of the parent into the template
extension: modifies the parent in place
Task: 1999528
closesodoo/odoo#33892
Signed-off-by: VincentSchippefilt <VincentSchippefilt@users.noreply.github.com>
Co-authored-by: Julien Mougenot <jum@odoo.com>
Co-authored-by: Lucas Perais <lpe@odoo.com>
