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

feat(common): add utility functions for xmlns attributes #173

Merged
merged 3 commits into from
May 17, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions packages/ast/lib/build-ast.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ const {
isArray,
assign
} = require("lodash");
const { findNextTextualToken } = require("@xml-tools/common");
const {
findNextTextualToken,
isXMLNamespaceKey,
getXMLNamespaceKeyPrefix
} = require("@xml-tools/common");

const { getAstChildrenReflective } = require("./utils");
const { DEFAULT_NS } = require("./constants");
Expand Down Expand Up @@ -256,13 +260,12 @@ function updateNamespaces(element, prevNamespaces = []) {
(result, attrib) => {
/* istanbul ignore else - Defensive Coding, not actually possible branch */
if (attrib.key !== invalidSyntax) {
const nsMatch = /^xmlns(?::([^:]+))?$/.exec(attrib.key);
if (nsMatch !== null) {
const prefix = nsMatch[1];
if (isXMLNamespaceKey(attrib.key, false) === true) {
const prefix = getXMLNamespaceKeyPrefix(attrib.key);
// TODO: Support un-defining namespaces (including the default one)
if (attrib.value) {
const uri = attrib.value;
if (prefix !== undefined) {
if (prefix !== "") {
result[prefix] = uri;
} else {
// default namespace
Expand Down
17 changes: 17 additions & 0 deletions packages/common/api.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,20 @@ export function findNextTextualToken(
tokenVector: IToken[],
prevTokEndOffset: number
): IToken | null;

/**
* Check if an xml attribute key is an xmlns key.
bd82 marked this conversation as resolved.
Show resolved Hide resolved
* @param key - the attribute key
* @param includeEmptyPrefix - should true be returned when there is no prefix (key === "xmlns:")
*/
export function isXMLNamespaceKey(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the term prefix be part of this name somehow?
I think that is the term used in the XML specs, perhaps instead of the key which is related to the attribute but we are dealing with a string here. (more like a tokenizer)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is only meaningful when related to xml attribute keys, a regular string isn't a namespace key, I wanted to make it clear

key: string,
includeEmptyPrefix: boolean
bd82 marked this conversation as resolved.
Show resolved Hide resolved
): boolean;

/**
* Get the attribute name, without its "xmlns:" prefix, from an xmlns attribute key.
* If the attribute key is not an xmlns key, undefined is returned.
* @param key - the attribute key
*/
export function getXMLNamespaceKeyPrefix(key: string): string | undefined;
8 changes: 7 additions & 1 deletion packages/common/lib/api.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
const { findNextTextualToken } = require("./find-next-textual-token");
const {
isXMLNamespaceKey,
getXMLNamespaceKeyPrefix
} = require("./xml-ns-key.js");

module.exports = {
findNextTextualToken: findNextTextualToken
findNextTextualToken: findNextTextualToken,
isXMLNamespaceKey: isXMLNamespaceKey,
getXMLNamespaceKeyPrefix: getXMLNamespaceKeyPrefix
};
38 changes: 38 additions & 0 deletions packages/common/lib/xml-ns-key.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// The xml parser takes care of validating the attribute name.
// If the user started the attribute name with "xmlns:" we can assume that
// they meant for it to be an xml namespace attribute.
// xmlns attributes explicitly can't contain ":" after the "xmlns:" part.
const namespaceRegex = /^xmlns(?<prefixWithColon>:(?<prefix>[^:]*))?$/;

function isXMLNamespaceKey(key, includeEmptyPrefix) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link doesn't work

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

if (typeof key !== "string") {
return false;
}
const matchArr = key.match(namespaceRegex);
if (matchArr === null) {
return false;
}
if (includeEmptyPrefix === true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (includeEmptyPrefix === true) { correctness depends on running after:
if (matchArr === null) { return false; }

should an else if be used to explicitly mark this relationship?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer not to, would it be enough to add a comment?

return true;
}
/* istanbul ignore next - defensive coding */
const groups = matchArr.groups || {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the || {} in JavaScript.

Because we know that in Node 10.0+ named capture groups are supported and that
the match succeed if we reached this line.

In TypeScript we used || {} to calm to compiler as the .groups property is defined as optional. so its not really defensive coding, we can prove it can't reach this branch so we don't actually need the branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll remove it

// In the case where key === "xmlns:", prefix will be empty and prefixWithColon will not be empty
return !(groups.prefixWithColon && !groups.prefix);
Copy link
Member

@bd82 bd82 May 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be simpler to have more distinct groups in the regExp? (no overlap)

  1. xmlns
  2. colon
  3. prefix

and then ask if 1 &2 exists (empty prefix case) or 1 & 2 & 3 exists (proper xmlns)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the condition a bit to make it clearer, let me know if you think I should still change this

}

function getXMLNamespaceKeyPrefix(key) {
bd82 marked this conversation as resolved.
Show resolved Hide resolved
if (typeof key !== "string") {
return undefined;
}
const matchArr = key.match(namespaceRegex);
if (matchArr === null) {
return undefined;
}
return (matchArr.groups && matchArr.groups.prefix) || "";
}

module.exports = {
isXMLNamespaceKey: isXMLNamespaceKey,
getXMLNamespaceKeyPrefix: getXMLNamespaceKeyPrefix
};
84 changes: 84 additions & 0 deletions packages/common/test/xml-ns-key-spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
const { expect } = require("chai");
const { isXMLNamespaceKey, getXMLNamespaceKeyPrefix } = require("../");

describe("The XML-Tools Common Utils", () => {
context("isXMLNamespaceKey", () => {
it("will return true for attribute name starting with xmlns:", () => {
expect(isXMLNamespaceKey("xmlns:a", true)).to.be.true;
expect(isXMLNamespaceKey("xmlns:a", false)).to.be.true;
});

it("will return true for attribute name starting with xmlns: that contains dots", () => {
expect(isXMLNamespaceKey("xmlns:a.b.c", true)).to.be.true;
expect(isXMLNamespaceKey("xmlns:a.b.c", false)).to.be.true;
});

it("will return true for the default namespace attribute", () => {
expect(isXMLNamespaceKey("xmlns", true)).to.be.true;
expect(isXMLNamespaceKey("xmlns", false)).to.be.true;
});

it("will return true for xmlns attribute without a name when includeEmptyPrefix is true", () => {
expect(isXMLNamespaceKey("xmlns:", true)).to.be.true;
});

it("will return false for xmlns attribute without a name when includeEmptyPrefix is false", () => {
expect(isXMLNamespaceKey("xmlns:", false)).to.be.false;
});

it("will return false for the non-xmlns attribute", () => {
expect(isXMLNamespaceKey("abc", true)).to.be.false;
expect(isXMLNamespaceKey("abc", false)).to.be.false;
});

it("will return false for non-xmlns attribute that starts with xmlns", () => {
expect(isXMLNamespaceKey("xmlnst", true)).to.be.false;
expect(isXMLNamespaceKey("xmlnst", false)).to.be.false;
});

it("will return false for attribute name starting with xmlns: that contains additional colons", () => {
tal-sapan marked this conversation as resolved.
Show resolved Hide resolved
expect(isXMLNamespaceKey("xmlns:a.b:c", true)).to.be.false;
expect(isXMLNamespaceKey("xmlns:a.b:c", false)).to.be.false;
});

it("will return false for undefined", () => {
expect(isXMLNamespaceKey(undefined, true)).to.be.false;
expect(isXMLNamespaceKey(undefined, false)).to.be.false;
});

it("will return false for null", () => {
expect(isXMLNamespaceKey(null, true)).to.be.false;
expect(isXMLNamespaceKey(null, false)).to.be.false;
});
});

context("getNamespaceKeyPrefix", () => {
it("will return the name without xmlns prefix for xmlns attribute with name", () => {
expect(getXMLNamespaceKeyPrefix("xmlns:abc")).to.eql("abc");
});

it("will return the name without xmlns prefix for xmlns attribute with name that contains dots", () => {
expect(getXMLNamespaceKeyPrefix("xmlns:abc.efg")).to.eql("abc.efg");
});

it("will return empty string when prefix is empty", () => {
expect(getXMLNamespaceKeyPrefix("xmlns:")).to.be.empty;
});

it("will return undefined when key does not start with xmlns", () => {
expect(getXMLNamespaceKeyPrefix("abc")).to.be.undefined;
});

it("will return undefined when key starts with xmlns but is not an xmlns key", () => {
expect(getXMLNamespaceKeyPrefix("xmlns*")).to.be.undefined;
});

it("will return undefined for undefined", () => {
expect(getXMLNamespaceKeyPrefix(undefined)).to.be.undefined;
});

it("will return undefined for null", () => {
expect(getXMLNamespaceKeyPrefix(null)).to.be.undefined;
});
});
});
4 changes: 2 additions & 2 deletions packages/simple-schema/lib/validators/unknown-attributes.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const { map, includes, forEach } = require("lodash");
const { isXMLNamespaceKey } = require("@xml-tools/common");
const { tokenToOffsetPosition } = require("./utils");

const NAMESPACE_ATTRRIBUTE_PATTERN = /^xmlns(:[^=]*)?$/;
/**
* @param {XMLElement} elem
* @param {XSSElement} schema
Expand All @@ -23,7 +23,7 @@ function validateUnknownAttributes(elem, schema) {
if (attrib.key !== null) {
if (
includes(allowedAttribNames, attrib.key) === false &&
NAMESPACE_ATTRRIBUTE_PATTERN.test(attrib.key) === false
isXMLNamespaceKey(attrib.key, true) === false
) {
issues.push({
msg: `Unknown Attribute: <${
Expand Down