Project

General

Profile

Actions

Bug #3967

closed

Change request cannot be accepted: multiline text cause merge incompatibility

Added by Vincent MEMBRÉ over 9 years ago. Updated over 9 years ago.

Status:
Released
Priority:
1
Category:
Web - Maintenance
Target version:
Severity:
UX impact:
User visibility:
Effort required:
Priority:
Regression:

Description

Sometimes, when creating a Change request about a Directive with multiple lines parameters (ie in enforce file content ) the change request cannot be validated due to a changed initial state.

But the initial state has not changed, the change request is not mergeable directly after the creating it.

the following examples throw the bug :

aaa
aaa
aaa

Sometimes the same text does not throw an error, but I can't undertand why


Related issues 2 (2 open0 closed)

Related to Rudder - Architecture #3972: Scala XML library accepts invalid XML charactersNewFrançois ARMANDActions
Related to Rudder - Bug #22208: Directives cannot be deleted because change request say state diverged even when there are no change request activatedIn progressNicolas CHARLESActions
Actions #1

Updated by Vincent MEMBRÉ over 9 years ago

  • Status changed from New to Pending technical review
  • Assignee changed from Vincent MEMBRÉ to François ARMAND

I finally understood what at stake here:

- Sometimes (not every time, still don't know why though, ), some line breaks are saved with '\r\n' (directly from LDAP)
- when saved in dabatase as an xml, the text stays (so still having the \r\n)
- The problem is : \r\n is not a valid xml line break, and when getting back this value as an xml it will be replaced by \n (see http://stackoverflow.com/questions/2265966/xml-carriage-return-encoding) the valid representation of a line break in a xml

- so all those texts were missing some characters (the \r, even if they look like they havent changed)

the solution is to transform \r before saving it to the databse to their valid xml value ->

I included debug logs for change requests, easing the process to understand what cause the merge conflict

Pull request here : https://github.com/Normation/rudder/pull/325

Actions #2

Updated by Vincent MEMBRÉ over 9 years ago

to repair a broken change request run this psql query :

update changerequest set content = xmlparse( document replace(XMLSERIALIZE (DOCUMENT content as text), chr(13), '
')) where id = x;
Actions #3

Updated by François ARMAND over 9 years ago

Are we sure we are escaping all invalid XML chars ? There is no point on escaping only "\r" if other chars could lead to the same behaviour.

In fact, there is in Scala scala.xml.Utility.escape / unescape. And we shoudl use it when we are saving in XML... And unescaping on deserialisation.

Whell, so the bug seems far more involved than what is shown here. I don't have a clear vision about how to adress that for now (inpact is huge... We use XML in a lot of place...)

Actions #4

Updated by François ARMAND over 9 years ago

  • Status changed from Pending technical review to 8
  • Assignee changed from François ARMAND to Vincent MEMBRÉ

Well, perhaps there is a bug on merge that should ignore control chars, and an other bug that is that we don't do clean XML serialisation / deserialisation...

Actions #5

Updated by Vincent MEMBRÉ over 9 years ago

Here is a stack overflow answer about invalid characters in XML: http://stackoverflow.com/questions/730133/invalid-characters-in-xml

Char       ::=      #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF]  /* any Unicode character, excluding the surrogate blocks, FFFE, and FFFF. */

\r is in this list #xD (\n too #xA)

Actions #6

Updated by Vincent MEMBRÉ over 9 years ago

List in previous comment is about valid characters,

so \r and \n are valid characters, but numerous xml parsers replace \r by an \n (see www.w3.org/TR/xml/#sec-common-syn)

To fix our problem here, we will normalize datas by serializing into xml datas from LDAP and deserializing then right after to get them into the same state than datas in change request.

Actions #7

Updated by Vincent MEMBRÉ over 9 years ago

  • Status changed from 8 to Pending technical review
  • Assignee changed from Vincent MEMBRÉ to François ARMAND
  • Pull Request set to https://github.com/Normation/rudder/pull/326
Actions #8

Updated by Vincent MEMBRÉ over 9 years ago

I added a commit to trim directive parameters(if an \r was inserted it was trimmed in LDAP but not in psql...)

François, if it's ok, don't merge so i can rebase it.

Actions #9

Updated by François ARMAND over 9 years ago

  • Status changed from Pending technical review to Discussion
  • Assignee changed from François ARMAND to Vincent MEMBRÉ

Some comments.

I really don't think that you have to restrict directive check to Directive. If we choose to embed all of (Technique, Directive, RootSpec) in the change request, we should allows to check on the three of them.

All other parts seem good.

Actions #10

Updated by Vincent MEMBRÉ over 9 years ago

  • Assignee changed from Vincent MEMBRÉ to François ARMAND

I updated the PR.

Actions #11

Updated by Vincent MEMBRÉ over 9 years ago

  • Status changed from Discussion to Pending technical review
Actions #12

Updated by Vincent MEMBRÉ over 9 years ago

  • Status changed from Pending technical review to Pending release
  • % Done changed from 0 to 100
Actions #13

Updated by Anonymous over 9 years ago

Actions #14

Updated by Nicolas PERRON over 9 years ago

  • Status changed from Pending release to Released
Actions #15

Updated by François ARMAND almost 6 years ago

Actions #16

Updated by François ARMAND about 1 month ago

  • Related to Bug #22208: Directives cannot be deleted because change request say state diverged even when there are no change request activated added
Actions

Also available in: Atom PDF