-
-
Notifications
You must be signed in to change notification settings - Fork 137
Accept IRI-valued quote URL properties #1043
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
base: 2.0-maintenance
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| links: | ||
| '#1015': https://github.com/fedify-dev/fedify/issues/1015 | ||
|
Comment on lines
+2
to
+3
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pinning the links in front matter is the right call, but #1043 is missing. Once the entry cites the pull request as well as the issue, this needs both: links:
'#1015': https://github.com/fedify-dev/fedify/issues/1015
'#1043': https://github.com/fedify-dev/fedify/pull/1043Separately, this change spans two published packages and only one of them has a fragment. The behaviour users observe lands in |
||
| --- | ||
| - Updated the `fedify:url` decoder to read `@id` when `@value` is absent, | ||
| allowing it to accept IRI-valued quote URL aliases (`_misskey_quote` or | ||
| `quoteUri`). Also widened its `dataCheck()` to accept both forms. | ||
| [[#1015] by Jang Hanarae] | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -308,12 +308,14 @@ const scalarTypes: Record<string, ScalarType> = { | |||||||||||||||||
| return `${v}.href`; | ||||||||||||||||||
| }, | ||||||||||||||||||
| dataCheck(v) { | ||||||||||||||||||
| return `typeof ${v} === "object" && "@value" in ${v} | ||||||||||||||||||
| && typeof ${v}["@value"] === "string" | ||||||||||||||||||
| && ${v}["@value"] !== "" && ${v}["@value"] !== "/"`; | ||||||||||||||||||
| return `typeof ${v} === "object" && | ||||||||||||||||||
| (("@value" in ${v} && typeof ${v}["@value"] === "string" && | ||||||||||||||||||
| ${v}["@value"] !== "" && ${v}["@value"] !== "/") || | ||||||||||||||||||
| ("@id" in ${v} && typeof ${v}["@id"] === "string" && | ||||||||||||||||||
| ${v}["@id"] !== "" && ${v}["@id"] !== "/"))`; | ||||||||||||||||||
|
Comment on lines
+311
to
+315
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping It generates nothing today. The guard is emitted only for properties with more than one range, and Second, If the decoder becomes tolerant the way I suggest below, this guard starts being emitted and its condition becomes load-bearing. It would then need to reject more than the empty string and |
||||||||||||||||||
| }, | ||||||||||||||||||
| decoder(v) { | ||||||||||||||||||
| return `new URL(${v}["@value"])`; | ||||||||||||||||||
| return `new URL(typeof ${v}["@value"] === "string" ? ${v}["@value"] : ${v}["@id"])`; | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The shape here is right, and it matches a precedent already on What it does not do yet is stop the crash the issue is about. I regenerated vocab.ts with this patch applied and fed
Row three is the same error text, from the same property, that #1015 reports. A sender that inlines the quoted object without an The reason The last row deserves its own note. codec.ts lines 428 to 438 already carry an Making the decoder tolerant needs a small change in the generator. Dropping the single-range special case altogether, the way --- a/packages/vocab-tools/src/class.ts
+++ b/packages/vocab-tools/src/class.ts
@@ -154,6 +154,17 @@ export async function* generateClasses(
isTemporalInstant,
} from "@fedify/vocab-runtime/temporal";\n`;
yield `
+function canParseIri(iri: string): boolean {
+ return URL.canParse(iri) || iri.startsWith("at://");
+}
+
+function parseIri(iri: string): URL {
+ return !URL.canParse(iri) && iri.startsWith("at://")
+ ? new URL("at://" + encodeURIComponent(iri.substring(5)))
+ : new URL(iri);
+}
+`;
+ yield `
function isValidLanguageTag(language: string): boolean {
--- a/packages/vocab-tools/src/type.ts
+++ b/packages/vocab-tools/src/type.ts
@@ -17,6 +17,12 @@ interface ScalarType {
dataCheck(variable: string): string;
decoder(variable: string, baseUrlVar: string): string;
+ /**
+ * Whether a value that fails `dataCheck()` should be skipped instead of
+ * being handed to `decoder()`. Set this for types decoded from untrusted
+ * remote input, where one malformed value must not fail the whole object.
+ */
+ skipUnparsable?: boolean;
}
@@ -309,14 +315,15 @@ const scalarTypes: Record<string, ScalarType> = {
dataCheck(v) {
return `typeof ${v} === "object" &&
- (("@value" in ${v} && typeof ${v}["@value"] === "string" &&
- ${v}["@value"] !== "" && ${v}["@value"] !== "/") ||
- ("@id" in ${v} && typeof ${v}["@id"] === "string" &&
- ${v}["@id"] !== "" && ${v}["@id"] !== "/"))`;
+ ((typeof ${v}["@value"] === "string" && canParseIri(${v}["@value"])) ||
+ (typeof ${v}["@id"] === "string" && canParseIri(${v}["@id"])))`;
},
decoder(v) {
- return `new URL(typeof ${v}["@value"] === "string" ? ${v}["@value"] : ${v}["@id"])`;
+ return `parseIri(
+ typeof ${v}["@value"] === "string" ? ${v}["@value"] : ${v}["@id"]
+ )`;
},
+ skipUnparsable: true,
},
@@ -597,6 +604,10 @@ export function getDecoder(
+export function skipsUnparsable(typeUri: string): boolean {
+ return scalarTypes[typeUri]?.skipUnparsable ?? false;
+}
+
export function getDataCheck(
--- a/packages/vocab-tools/src/codec.ts
+++ b/packages/vocab-tools/src/codec.ts
@@ -6,11 +6,13 @@ import {
areAllScalarTypes,
emitOverride,
getAllProperties,
+ getDataCheck,
getDecoder,
getDecoders,
getEncoders,
getSubtypes,
isCompactableType,
+ skipsUnparsable,
} from "./type.ts";
@@ -441,7 +443,13 @@ export async function* generateDecoder(
yield `
const decoded =
`;
+ const lenient = property.range.length == 1 &&
+ skipsUnparsable(property.range[0]);
if (property.range.length == 1) {
+ if (lenient) {
+ yield getDataCheck(property.range[0], types, "v");
+ yield " ? ";
+ }
yield getDecoder(
property.range[0],
@@ -449,6 +457,7 @@ export async function* generateDecoder(
"options",
`(values["@id"] == null ? options.baseUrl : new URL(values["@id"]))`,
);
+ if (lenient) yield " : undefined";
} else {
@@ -462,7 +471,7 @@ export async function* generateDecoder(
yield `
;
`;
- if (property.range.length > 1) {
+ if (property.range.length > 1 || lenient) {
yield `
if (typeof decoded === "undefined") {With that applied, every throwing row above becomes Sharing |
||||||||||||||||||
| }, | ||||||||||||||||||
| }, | ||||||||||||||||||
| "fedify:publicKey": { | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -809,6 +809,40 @@ test("Note.quoteUrl", async () => { | |
| deepStrictEqual(loaded3.quoteUrl, new URL("https://example.com/object3")); | ||
| }); | ||
|
|
||
| test("Note.quoteUrl (IRI-typed alias terms)", async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a real regression test. I reverted type.ts to its base revision, forced a regeneration and confirmed it fails with Two gaps worth closing:
Once unparsable values are dropped instead of thrown, this is also the right place to assert it: a |
||
| const jsonLd: Record<string, unknown> = { | ||
| "@context": [ | ||
| "https://www.w3.org/ns/activitystreams", | ||
| { | ||
| fedibird: "http://fedibird.com/ns#", | ||
| misskey: "https://misskey-hub.net/ns#", | ||
| _misskey_quote: { | ||
| "@id": "misskey:_misskey_quote", | ||
| "@type": "@id", | ||
| }, | ||
| quoteUri: { | ||
| "@id": "fedibird:quoteUri", | ||
| "@type": "@id", | ||
| }, | ||
| }, | ||
| ], | ||
| id: "https://example.com/notes/1", | ||
| type: "Note", | ||
| _misskey_quote: "https://example.com/notes/quoted", | ||
| quoteUri: "https://example.com/notes/quoted2", | ||
| }; | ||
|
|
||
| const loaded = await Note.fromJsonLd(jsonLd); | ||
| deepStrictEqual(loaded.quoteUrl, new URL("https://example.com/notes/quoted")); | ||
|
|
||
| delete jsonLd._misskey_quote; | ||
| const loaded2 = await Note.fromJsonLd(jsonLd); | ||
| deepStrictEqual( | ||
| loaded2.quoteUrl, | ||
| new URL("https://example.com/notes/quoted2"), | ||
| ); | ||
| }); | ||
|
|
||
| test("Key.publicKey", async () => { | ||
| const jwk = { | ||
| kty: "RSA", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This describes the generator rather than the change.
fedify:urlanddataCheck()are vocab-tools internals that no user of@fedify/vocabever sees, and the second sentence describes something that currently generates no code at all. Entries here should say what broke for users and when.Two smaller things. Neighbouring bug fix entries open with “Fixed”, not “Updated”. And the marker should carry the pull request number alongside the issue, the way the
@fedify/redisentry above carries[[#1028], [#1034]].Something along these lines:
Note the two spaces after each sentence, which the surrounding entries use. Edit changes.d/vocab/iri-valued-quote-url.md and run
sacho syncrather than editing this file by hand.