mirror of https://github.com/Squidex/squidex.git
Browse Source
* Pin observable scripting behaviour before the Jint upgrade The Jint upgrade that follows re-arms three engine-wide inline cache gates. Each of those changes is only worth making if it is observably identical, so pin the behaviour first, against Jint 4.8.0: - null propagation: undeclared identifier reads, nullish property chains, calls over a nullish base, calls of a non-callable member (which return the base), and that ordinary member reads/calls are unaffected; - enum values crossing into script as their member name, including a [Flags] combination and an enum member of a wrapped CLR object; - JSON objects projected into script: own key order, JSON.stringify, for..in, mutation (add/replace/delete) and the round trip back to JsonValue; - content field objects: `in`, hasOwnProperty, propertyIsEnumerable, Object.keys, spread, JSON.stringify, and that all of them follow deletes and additions; - the context object: key enumeration, `in`, typeof per value, reads, write-through to ScriptVars and delete. All 156 tests in Operations/Scripting pass unchanged on 4.8.0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uV6H9cTntzsoKiaJRBn4f * Upgrade Jint from 4.8.0 to 4.15.3 Package provenance verified after restore: source is https://api.nuget.org/v3/index.json and the nuspec repository commit is a304aa5dacd340e2a5ff51e1ea0c465e38e50aa8, the v4.15.3 tag. Acornima moves to 1.6.2 transitively; ParseErrorException, ScriptPreparationException and JintException, which JintScriptEngine.MapException switches on, all still exist, and so do Engine.Constraints.Reset, Options.Constraints.PromiseTimeout, AllowClrWrite, EvaluateAsync(in Prepared<Script>, CancellationToken), ObjectWrapper.Create and the ObjectInstance virtuals the ContentWrapper family overrides. No source change is needed for the upgrade itself: the whole backend solution builds warning-clean and all 156 tests in Operations/Scripting, including the behaviour pins added in the previous commit, pass unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uV6H9cTntzsoKiaJRBn4f * Verify Jint's host contracts on every test run The scripting integration defines several Jint extension points: the ContentWrapper objects override GetOwnProperty, and the engine trusts the answer without re-verifying it on the hot path. A hook that contradicts another therefore fails silently in production - a key vanishes from every enumeration, or a read resolves on the prototype for a property that exists - which is the class of bug no assertion in this repository would catch. Jint 4.15.3 exposes its host-contract verifiers to the shipped Release package through an AppContext switch, where before they were compiled out unless you built the engine from source in Debug. A module initializer sets it for this test assembly, so the verifiers run against the same NuGet package production uses and report a violation as an ordinary test failure. It must be set before the first use of any Jint type, which is exactly what a module initializer guarantees. Confirmed live rather than assumed: with a deliberately wrong ProbeOwnProperty the run fails with "ContentFieldObject.ProbeOwnProperty answered 'iv' with Missing but its GetOwnProperty reports Enumerable". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uV6H9cTntzsoKiaJRBn4f * Declare the reference resolver's interests to re-arm the read caches Registering an IReferenceResolver without interests gives it ReferenceResolverInterests.All, and two of those flags - ObjectPropertyBase and PrimitivePropertyBase - are the gate on the non-computed member-read inline caches, the dense-array indexed-read lane and the member-call callee lane. With All declared, every property read in every script has to be routed through a Reference so the resolver gets offered the base, and all three lanes stay off for the whole engine. NullPropagation.TryPropertyReference returns false for every base that is not null or undefined, so on those two situations the engine consulting it can never change the result. Declaring only the three situations the resolver actually answers - NullishPropertyBase, UnresolvableReference and NonCallableCallee - is therefore observably identical and re-arms all three lanes. Interests are documented as a subscription filter and not a promise: a situation that is not subscribed to behaves exactly as if no resolver were registered. Jint also ships a built-in NullPropagatingReferenceResolver, which is deliberately NOT adopted here: it declines unresolvable identifiers and non-callable callees, where this resolver answers both, so swapping it in would turn an undeclared-name read and a call on a nullish chain into errors for existing tenant scripts. The behaviour pins from the first commit cover exactly those edges and all 156 tests in Operations/Scripting still pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uV6H9cTntzsoKiaJRBn4f * Declare the object converter's types and convert enums natively An IObjectConverter registered without declaring the CLR types it handles can be handed anything, so the engine has to assume every wrapped CLR member read might reach it and disables the compiled interop member-read lane engine-wide. JintObjectConverter handles a closed set, so declare it: matching is by assignability, which keeps IUser covering every implementation. The converter is still offered every value - the declaration only lets the engine keep the fast lane for members whose declared type could never produce a handled value, and it errs towards claiming (a member typed `object` is always claimed). The Enum branch is dropped in favour of Options.Interop.EnumConversion = EnumConversionMode.String, which Jint documents as the member name "as produced by object.ToString()", including the comma-separated combination for a [Flags] value and the numeric value rendered as a string for a value with no name - verbatim what the branch did. The write direction keeps accepting both the name and the number. Handling enums natively rather than through the converter also keeps one more declared type off the list, so more members stay on the fast lane. Pinned by Should_convert_enum_to_name, Should_convert_flags_enum_to_names and Should_convert_enum_member_of_wrapped_object_to_name, which pass before and after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uV6H9cTntzsoKiaJRBn4f * Project JSON objects into script as shape-mode objects JsonMapper built every JSON object it projects into script as an instance of a private ObjectInstance subclass that existed only to be instantiable. A host subclass can never carry the engine's shape-mode storage flag, so each of those objects - all of ctx.data's leaf objects, and every JsonValue var - sat permanently outside the own-property inline caches, and a script reading the same property across a batch of content items re-resolved it every time. JsObject.CreateFromEntries builds the same object through the hidden class machinery instead: repeated calls presenting the same key sequence, which every content item of one schema does, share an interned hidden class, so those reads stay monomorphic. The result is documented as indistinguishable from the equivalent object literal - same own key order, same configurable/enumerable/writable data properties - and anything the representation cannot express (a digit-leading key, a very wide object) falls back to the ordinary dictionary representation rather than to different behaviour. That fallback is silent, which is why the shaping is asserted rather than assumed: Engine.Advanced.HasSharedShape is the supported predicate for it, and JsonMapperTests pins that the projected object and its nested objects answer true. Building them as a host subclass again would fail that test. Three smaller fixes in the same file: - the reverse direction allocated a string key per array element (a.Get(i.ToString(...))); the indexed accessor reads the dense backing directly and keeps the prototype walk for a modified array; - JsNumber.Create reuses cached instances for small integers where new JsNumber always allocated; - JsString.Create, public since 4.15.3, interns the empty and single character strings where new JsString always allocated. Pinned by the projection tests added first - own key order, JSON.stringify, for..in, mutation including delete and add, and the round trip back to JsonValue - which pass before and after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uV6H9cTntzsoKiaJRBn4f * Answer field existence questions without mapping the field value ContentFieldProperty is a CustomJsValue descriptor whose CustomValue maps the stored JsonValue to a JsValue on first read. Existence and enumerability questions never need that value, but they used to pay for it: `in`, hasOwnProperty, propertyIsEnumerable, Object.keys/values/entries, Object.assign, object spread and JSON.stringify all reached the object through GetOwnProperty, which materializes the descriptor whose value the caller then reads or discards. Jint lets a host answer those questions directly through ProbeOwnProperty. The override deliberately mirrors GetOwnProperty line for line, minus the descriptor: same initialization, same toJSON exclusion, same lookup, and the enumerable flag read off the descriptor rather than off its value. The engine trusts the probe without re-verifying it on the hot path, so a wrong Missing would silently drop the key from every enumeration above - which is why the two are kept adjacent in the file, pinned by tests covering `in`, hasOwnProperty, propertyIsEnumerable, Object.keys, spread and JSON.stringify plus a delete and an add, and checked on every test run by the host-contract verification enabled earlier in this branch. ContentDataObject deliberately does not get the same override: its GetOwnProperty auto-creates a field for any name probed, so a probe that agreed with it at the same instant would have to do the same, and that quirk is pre-existing tenant-visible behaviour this change has no business altering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uV6H9cTntzsoKiaJRBn4f * Map script variables on first read instead of eagerly Every variable was mapped when the engine was set up, once per evaluation, whether or not the script ever looked at it - WritableContext did it in its constructor for the ctx path, and Engine.SetValue did it per variable for the non-context path. Some of those mappings are not cheap: a user variable walks and groups every claim, a content data variable builds a wrapper. A typical script reads a handful of the variables available to it. Both paths now defer the mapping to the first read of the value, through the two APIs Jint 4.15.3 added for exactly this: - PropertyDescriptor.CreateLazy for the ctx object. Unlike a hand-written CustomJsValue descriptor it drops the flag once the value exists, so the descriptor rejoins the write inline cache instead of paying the indirection for the rest of its life. - Engine.Advanced.AddLazyGlobal for the non-context path. The options-time AddLazyGlobal could not serve it - the variables are only known after the engine has been built - and the descriptor a host could install itself is declined by the global-identifier cache. The Advanced overload is documented as being for exactly this case, and its factory may capture engine-affine state. In both cases the property itself is installed eagerly, so nothing about the shape changes: key order, enumeration, `in`, Object.getOwnPropertyNames, delete and the write-through to ScriptVars behave exactly as before, which is what the tests pin - including a counting principal that proves the mapping has not run for a variable the script never mentions, and has run for one it reads. MapVariable reproduces Engine.SetValue's special case for a CLR type so a deferred variable cannot project differently. One edge is worth recording: Engine.SetValue writes through [[Set]] while AddLazyGlobal replaces the descriptor, so a variable named after a non-writable built-in global (undefined, NaN, Infinity) would now shadow it where it was previously ignored. ScriptVars keys are domain names, so this is not reachable in practice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uV6H9cTntzsoKiaJRBn4f * Rewrite the code comments in plainer language The comments explained the change in Jint's own vocabulary - inline caches, shape mode, descriptors, lanes - which is not vocabulary this repository uses. Say what each change does and why it is worth it instead, and name a Jint concept only where the reader has to look it up anyway. No behaviour change: comments and XML docs only, plus one short comment on the enum conversion option. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>master
Marko Lahma
3 weeks ago
committed by
GitHub
GitHub
12 changed files with 829 additions and 26 deletions
@ -0,0 +1,52 @@ |
|||||
|
// ==========================================================================
|
||||
|
// Squidex Headless CMS
|
||||
|
// ==========================================================================
|
||||
|
// Copyright (c) Squidex UG (haftungsbeschraenkt)
|
||||
|
// All rights reserved. Licensed under the MIT license.
|
||||
|
// ==========================================================================
|
||||
|
|
||||
|
using Jint; |
||||
|
using Jint.Native.Object; |
||||
|
using Squidex.Domain.Apps.Core.Scripting.Internal; |
||||
|
using Squidex.Infrastructure.Json.Objects; |
||||
|
|
||||
|
namespace Squidex.Domain.Apps.Core.Operations.Scripting; |
||||
|
|
||||
|
public class JsonMapperTests |
||||
|
{ |
||||
|
[Fact] |
||||
|
public void Should_map_json_objects_into_a_shared_shape() |
||||
|
{ |
||||
|
var engine = new Engine(o => o.Strict()); |
||||
|
|
||||
|
var mapped = (ObjectInstance)JsonMapper.Map(CreateJson(), engine); |
||||
|
var nested = (ObjectInstance)mapped.Get("nested"); |
||||
|
|
||||
|
// Sharing the layout is what makes reading properties of many content items fast. It only affects
|
||||
|
// performance and never behavior, which is why it is asserted here: building these objects with a
|
||||
|
// custom ObjectInstance class again would silently undo it and no other test would notice.
|
||||
|
Assert.True(engine.Advanced.HasSharedShape(mapped)); |
||||
|
Assert.True(engine.Advanced.HasSharedShape(nested)); |
||||
|
} |
||||
|
|
||||
|
[Fact] |
||||
|
public void Should_share_the_shape_between_objects_of_the_same_shape() |
||||
|
{ |
||||
|
var engine = new Engine(o => o.Strict()); |
||||
|
|
||||
|
var first = (ObjectInstance)JsonMapper.Map(CreateJson(), engine); |
||||
|
var second = (ObjectInstance)JsonMapper.Map(CreateJson(), engine); |
||||
|
|
||||
|
Assert.True(engine.Advanced.HasSharedShape(first)); |
||||
|
Assert.True(engine.Advanced.HasSharedShape(second)); |
||||
|
} |
||||
|
|
||||
|
private static JsonValue CreateJson() |
||||
|
{ |
||||
|
return JsonValue.Create( |
||||
|
new JsonObject() |
||||
|
.Add("name", JsonValue.Create("squidex")) |
||||
|
.Add("count", JsonValue.Create(3)) |
||||
|
.Add("nested", JsonValue.Create(new JsonObject().Add("flag", JsonValue.True)))); |
||||
|
} |
||||
|
} |
||||
@ -0,0 +1,33 @@ |
|||||
|
// ==========================================================================
|
||||
|
// Squidex Headless CMS
|
||||
|
// ==========================================================================
|
||||
|
// Copyright (c) Squidex UG (haftungsbeschraenkt)
|
||||
|
// All rights reserved. Licensed under the MIT license.
|
||||
|
// ==========================================================================
|
||||
|
|
||||
|
using System.Runtime.CompilerServices; |
||||
|
|
||||
|
namespace Squidex.Domain.Apps.Core.TestHelpers; |
||||
|
|
||||
|
/// <summary>
|
||||
|
/// Turns on Jint's self checks for this test assembly.
|
||||
|
/// </summary>
|
||||
|
/// <remarks>
|
||||
|
/// Our ContentWrapper classes and the object converter implement Jint extension points where Jint relies on
|
||||
|
/// our answers being consistent, without checking them - checking would cost as much as the shortcut saves.
|
||||
|
/// A mistake there is silent in production: a key can disappear from Object.keys, or a converted type can be
|
||||
|
/// skipped. With this switch on Jint verifies the answers and throws on the first mismatch, so a mistake
|
||||
|
/// fails a test instead.
|
||||
|
/// <para>
|
||||
|
/// The switch has to be set before the first Jint type is used, which is what the module initializer
|
||||
|
/// guarantees. It stays off in production, where the checks would only cost performance.
|
||||
|
/// </para>
|
||||
|
/// </remarks>
|
||||
|
internal static class JintHostContractVerification |
||||
|
{ |
||||
|
[ModuleInitializer] |
||||
|
internal static void Enable() |
||||
|
{ |
||||
|
AppContext.SetSwitch("Jint.EnableHostContractVerification", true); |
||||
|
} |
||||
|
} |
||||
Loading…
Reference in new issue