Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is the target compatible with the new NLog extension configuration? #307

Closed
davidgeary opened this issue Jun 16, 2022 · 32 comments
Closed
Assignees
Labels

Comments

@davidgeary
Copy link
Contributor

Bug description
Using the latest version of NLog (5.x), extension assemblies must now be explicitly specified, either via the <extensions> element or by suffixing the type property of the <target> element, as described in PR 295.

However, when the type property is suffixed with the assembly name, a schema warning is displayed in Visual Studio.

Note that the warning does not prevent compilation (unless your project has Treat warnings as errors set to true). Regardless, a workaround is to specify the assembly in the <extensions> element instead, rather than in the target's type.

Expected behavior
No warning to be displayed.

Actual behavior
A schema warning is displayed:

The attribute 'type' has an invalid value 'sl:Syslog, NLog.Targets.Syslog' according to its schema type 'QName' - The ',' character, hexadecimal value 0x2C, cannot be included in a name.

Visual Studio screenshot:

nlog schema warning

To reproduce

  1. Create an NLog configuration file, e.g. as per the documentation example
  2. Remove the <extensions> element
  3. Update the <target> element opening tag from:
<target xsi:type="Syslog" name="cee-udp">

to

<target xsi:type="Syslog, NLog.Targets.Syslog" name="cee-udp">

Additional context

  • NLog version: 5.0.1
  • NLog.Targets.Syslog version 7.0.0
@snakefoot
Copy link
Contributor

snakefoot commented Jun 18, 2022

Looks like I was not thorough enough, when adding support for embedded assembly-name in type-alias. I liked the dotnet-syntax of specifying the assembly-name. But seems it is not compatible with QName-format.

Maybe adding support for assembly-name as QName-prefix like this:

<target xsi:type="NLog.Targets.Syslog:Syslog" name="cee-udp">

See also: NLog/NLog#4979

@snakefoot
Copy link
Contributor

Curious is the XML Schema validation is still able to validate properties for the SysLog-target when using NLog.Targets.Syslog: as prefix?

@snakefoot
Copy link
Contributor

@davidgeary In your example you use xsi:type="sl:syslog", any reason why you include the sl:-prefix ?

@snakefoot
Copy link
Contributor

Maybe it comes from these examples:

https://github.com/luigiberrettini/NLog.Targets.Syslog/blob/master/src/TestAppWithTUI/NLog.config
https://github.com/luigiberrettini/NLog.Targets.Syslog/blob/master/src/TestAppWithGUI/NLog.config

Curious what is expected from xsi:type="sl:syslog" instead of just xsi:type="syslog" ?

@davidgeary
Copy link
Contributor Author

In your example you use xsi:type="sl:syslog", any reason why you include the sl:-prefix ?

The precise reason is lost in the mists of time, but best guess is that I just copied it from an example somewhere :-)

I've just tried replacing sl: with NLog.Targets.Syslog: in the type property and it works the same as long as the assembly is still included after the type name. Just using the prefix without appending the assembly name fails to load it, but that's probably to be expected, since the <target> element is NLog-specific (rather than part of the XML std), so the format of the type property will surely need to conform to the formats supported by NLog?

AFAICS, the only place they mention the new capability of including the assembly name is in the list of NLog 5.0 changes:

This means custom targets requires explicit <add assembly="NLog.MyAssembly" />, or use the new ability to specify type with assembly-name: <target type="MyTarget, NLog.MyAssembly" name="hello" />.

This would suggest that the assembly must follow the type, separated by a comma.

(Was about to suggest getting clarification from NLog, but have just seen the issue you've raised, so will add a comment to that.)

@snakefoot
Copy link
Contributor

snakefoot commented Jun 20, 2022

Thank you for confirming that intellisense also works for you when using prefix NLog.Targets.Syslog:. And also thank you for confirming that there is no clear reason for using sl: as prefix.

Now just need to decide if it should be NLog 5.0.2 or NLog 5.1 for using xsi:type qname-prefix as hint for assembly-loading (So XSD-intellisense will work together with assembly-loading-hint)

@luigiberrettini luigiberrettini changed the title XML schema warning issued when specifying extension assembly via target's type property Is the target compatible with the new NLog extension configuration? Jun 21, 2022
@luigiberrettini
Copy link
Owner

Curious what is expected from xsi:type="sl:syslog" instead of just xsi:type="syslog"?

The sl: prefix is used to comply with the specifications by providing a QName as the xsi:type attribute expects.

@luigiberrettini
Copy link
Owner

Closing since the misbehavior is not caused by the target.

@snakefoot
Copy link
Contributor

snakefoot commented Jun 21, 2022

The sl: prefix is used to comply with the specifications by providing a QName as the xsi:type attribute expects.

Pretty sure the QName-prefix is optional.

@luigiberrettini
Copy link
Owner

Pretty sure the QName-prefix is optional

Yes, like for attribute names.
I just answered to clarify the doubts about its usage and to highlight the fact is has to be supported regardless of being optional or not.

@snakefoot
Copy link
Contributor

snakefoot commented Jun 21, 2022

highlight the fact is has to be supported regardless of being optional or not.

Yes currently NLog puts a lot of effort into stripping away the prefix, so was just curious what benefit the prefix had.

Was maybe thinking to support this format xsi:type="NLog.Targets.Syslog:Syslog", where NLog will use the prefix as Assembly-loading-hint.

@davidgeary
Copy link
Contributor Author

Closing since the misbehavior is not caused by the target.

But this issue wasn't about the target specifically; it was to do with the XML schema, which identifies the new format as invalid. (The original title of this issue was "XML schema warning issued when specifying extension assembly via target's type property".)

The warning incorrectly gives the impression that including the assembly name in the type property is invalid, when in reality, it is the schema that is wrong, since NLog no longer requires the type property to be a QName.

@snakefoot
Copy link
Contributor

snakefoot commented Jun 22, 2022

But this issue wasn't about the target specifically; it was to do with the XML schema, which identifies the new format as invalid.

The xsi:type is not part of the XML-Schema from NLog.xsd or NLog.Targets.Syslog.xsd.

The problem is not the XML-schema, but the NLog-team having broken the XSD-xsi-standard, by introducing a non-standard format which doesn't work with the xsi:type-attribute.

Guess we need more space-cowboys from the 2000's that knows XML / XSD / XSLT by heart on the NLog-team, but until that happens, then we just have to go along with what we have, and repair as we go. See also NLog/NLog#4979

@luigiberrettini
Copy link
Owner

It seems from Wikipedia that the QName syntax is Prefix:LocalPart where both Prefix and LocalPart are NCNames.

An NCName value follows these rules:

  • it does not contain a colon
  • it must consist exclusively of letters, digits, ideographs, underscores, hyphens, periods
  • it cannot start with a digit, an hyphen, the period

This means that NLog should support one of the two options below, where sl:Syslog_NLog.Targets.Syslog is just a possible choice to comply with NCName syntax.

OPTION 1

<?xml version="1.0" encoding="utf-8"?>
<nlog xmlns="http://www.nlog-project.org/schemas/NLog.xsd"
      xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
      xmlns:sl="http://www.nlog-project.org/schemas/NLog.Targets.Syslog.xsd">
    <extensions>
        <add assembly="NLog.Targets.Syslog"/>
    </extensions>
    <targets>
        <target xsi:type="sl:Syslog" name="syslog-tgt">
            <!-- ... -->
        </target>
    </targets>
    <!-- ... -->
</nlog>

OPTION 2

<?xml version="1.0" encoding="utf-8"?>
<nlog xmlns="http://www.nlog-project.org/schemas/NLog.xsd"
      xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
      xmlns:sl="http://www.nlog-project.org/schemas/NLog.Targets.Syslog.xsd">
    <targets>
        <target xsi:type="sl:Syslog_NLog.Targets.Syslog" name="syslog-tgt">
            <!-- ... -->
        </target>
    </targets>
    <!-- ... -->
</nlog>

@snakefoot
Copy link
Contributor

snakefoot commented Jun 22, 2022

@luigiberrettini Any reason why xsi:type="NLog.Targets.Syslog:Syslog" is bad ? (Since XSD-intellisense and XSD-validation will continue to work with that format)

@luigiberrettini
Copy link
Owner

luigiberrettini commented Jun 22, 2022

Because prefix should be an XML namespace declared in the file like xsi and sl and using NLog.Targets.Syslog as a prefix would not allow to specify the sl namespace.

In my opinion the goal should be both complying with specs and avoiding validation warnings/errors.

@snakefoot
Copy link
Contributor

snakefoot commented Jun 22, 2022

When using <target xsi:type="sl:Syslog_NLog.Targets.Syslog" name="syslog-tgt"> then XSD-intellisense and XSD-Validation stops working.

But with xsi:type="NLog.Targets.Syslog:Syslog" then everything is great. Again have no clue about what meaning the "prefix" has since it seems to be ignored by XSD-tooling.

@luigiberrettini
Copy link
Owner

luigiberrettini commented Jun 22, 2022

Ok then we need to find something that respects the specs and can be validated with a proper XML validator and then see if the Visual Studio validator works properly.

@snakefoot
Copy link
Contributor

snakefoot commented Jun 22, 2022

What about xsi:type="NLog.Targets.Syslog:Syslog" ? (Again it is only an option that you can use, not something that is required if you like to use <extensions> instead)

@luigiberrettini
Copy link
Owner

As I said that does not respects the specs.

Having as a goal to remove warnings will result in somebody else opening an issue because it is not XML compliant and not solving both is just saying, use another option, in which case this could have been the reply to this issue also.

@snakefoot
Copy link
Contributor

snakefoot commented Jun 22, 2022

How does the QName-prefix NLog.Targets.Syslog: break the specs and the tooling ?

@luigiberrettini
Copy link
Owner

As I wrote above the prefix must be an XML namespace and it is not.

I found out that xsi:type="sl:Syslog_NLog.Targets.Syslog" works but the NLog XSD needs to be changed: sl:* are identifiers looked up into the XSD.

@luigiberrettini
Copy link
Owner

The best solution would be to use the power of XML adding an assembly optional attribute:

<?xml version="1.0" encoding="utf-8"?>
<nlog xmlns="http://www.nlog-project.org/schemas/NLog.xsd"
      xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
      xmlns:sl="http://www.nlog-project.org/schemas/NLog.Targets.Syslog.xsd">
    <targets>
        <target xsi:type="sl:Syslog" assembly="NLog.Targets.Syslog" name="syslog-tgt">
            <!-- ... -->
        </target>
    </targets>
    <!-- ... -->
</nlog>

@snakefoot
Copy link
Contributor

assembly is not a "reserved"-word like xsi:type

@luigiberrettini
Copy link
Owner

I do not know if it is reserved, to me you can use whatever word you prefer: you just need to add it in the NLog XSD specification so that it is recognized as attribute of the target element.

@snakefoot
Copy link
Contributor

snakefoot commented Jun 22, 2022

Yes that is also a possible way for making assembly-loading-hint work with xsi:type-attribute without validation-errors.

Though I prefer xsi:type="NLog.Targets.Syslog:Syslog" (Since NLog.config never promised to follow XSD-standard, but like the help from the tooling with validation and intellisense)

@luigiberrettini
Copy link
Owner

The important thing is just to declare it in the docs, then you can even diverge a lot more from the standard.

@luigiberrettini
Copy link
Owner

Still I believe that a separate attribute in the XSD Target abstract complex type would be cleaner, more efficient than splitting for parsing purposes and could allow to deprecate the extensions altogether.

@snakefoot
Copy link
Contributor

snakefoot commented Jun 22, 2022

<extensions> are still needed for layout-renderers extensions when parsing NLog Layout. Having the assembly-loading-hint was just a random idea to make it easier for NLog-users to use NLog-target-extensions, but also a bridge to allow NLog-core-project to eject NLog-targets that was specific to Microsof Windows into their own nuget-packages.

Can now see that my initial idea was a little better (xsi:type="sl:NLog.Targets.Syslog.Syslog") according to XML-standards, and it was only the suggestion from @ThomasArdal that made it break XML-standards for QName. But XSD-intellisense would still be broken

@luigiberrettini
Copy link
Owner

luigiberrettini commented Jun 22, 2022

One not so nice solution but working and complying with specs:

<?xml version="1.0" encoding="utf-8"?>
<nlog xmlns="http://www.nlog-project.org/schemas/NLog.xsd"
      xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
      xmlns:NLog.Targets.Syslog="http://www.nlog-project.org/schemas/NLog.Targets.Syslog.xsd">
    <targets>
        <target xsi:type="NLog.Targets.Syslog:Syslog" name="syslog-tgt">
            <NLog.Targets.Syslog:layout xsi:type="SimpleLayout" text="${message}" />
            <!-- ... -->
        </target>
    </targets>
    <!-- ... -->
</nlog>

@luigiberrettini
Copy link
Owner

xsi:type="sl:NLog.Targets.Syslog.Syslog"

Considering that you want to support both extensions plus target name and no extensions plus assembly plus target name there is no solution working since target XSDs only have one or the other.

I have Syslog in mine and any change would imply me changing the name in the XSD, making the XSD NuGet package useless unless you force all target implementers to always use the full name.

@snakefoot
Copy link
Contributor

snakefoot commented Jun 22, 2022

The idea was not to force anything, but just to make it easier to provide an assembly-loading-hint with the type-alias.

But see that you have to navigate careful in the woods of XML and xmlns. Guess @davidgeary should stay away from using xsi:type with assembly-loading-hint (And stick with <extensions>), until a white knight arrives and clears everything up,.

Alternative replace xsi:type="sl:Syslog, NLog.Targets.Syslog with type="Syslog, NLog.Targets.Syslog", but then loose XML-validation and XML-intellisense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants