Skip to content

Conversation

@StefanApfel-Bentley
Copy link
Contributor

@StefanApfel-Bentley StefanApfel-Bentley commented Oct 2, 2025

Adds the incremental schema locaters to IModelDb and IModelConnection schemacontext as default locater.
Also introduces an internal api to enable/disable incremental schema loading through a helper method.

enableIncrementalSchemaLoading(true);
enableIncrementalSchemaLoading(false);

Default is currently set to false.

@pmconne
Copy link
Member

pmconne commented Oct 2, 2025

Can you please explain in the PR or issue description why we are making this change?

Also introduces an internal api to enable/disable incremental schema loading through a helper method.

Why is it internal? Who is expected to call it? AFAICT, right now it is only used by tests.

@StefanApfel-Bentley
Copy link
Contributor Author

StefanApfel-Bentley commented Oct 2, 2025

Can you please explain in the PR or issue description why we are making this change?

We added incremental schema loading as additional way to load schemas incrementally instead of all at once as JSON documents. After some testing now, we want to add the incremental schema locaters as default locaters on the schemacontexts of all IModels and IModelConnections replacing the default schemaProps loader.

More information: Loading schemas incrementally via ECSql

Also introduces an internal api to enable/disable incremental schema loading through a helper method.

Why is it internal? Who is expected to call it? AFAICT, right now it is only used by tests.

It's internal because we don't want to get it added to the public API, only internal components (within itwinjs) shall enable it if they want to use it now. One candidate for that is presentation (who also requested the change in the first place). I can imagine after some additional test runs of several components, we might switch the default from disabled to enabled.

@ColinKerr
Copy link
Member

There as got to be some better way to do this. Outside the scope of this one case we are going to need the ability to have some configuration to flag on/off code. Should each case be handled individually or should we have some pattern(s) to follow?

@aruniverse


// Global options for incremental schema loading.
const schemaLoadingOptions = {
isEnabled: false,
Copy link
Member

Choose a reason for hiding this comment

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

from the name of the top level obj and this property, i would not have guessed this was wrt to incremental schema loading.

i dont think i love this approach of a psuedo global var that is checked in both frontend and backend. is there a reason why both frontend and backend need to respect this, and both of them (frontend and backend) could decide indepently which approach (incremental vs not) to use?

@aruniverse aruniverse requested a review from grigasp October 8, 2025 20:51
@grigasp
Copy link
Member

grigasp commented Oct 9, 2025

It's internal because we don't want to get it added to the public API, only internal components (within itwinjs) shall enable it if they want to use it now. One candidate for that is presentation (who also requested the change in the first place)

Our components that would benefit from incremental schema loading are outside of this repo, so any API they need to call should be public.

However, why not just have it always enabled?

I don't really like the idea of a single component deciding to change global state that affects the whole application. IMO, if for some reason we can't have that enabled by default, then it should be a configuration option at the application level.

@StefanApfel-Bentley
Copy link
Contributor Author

I'm currently trying to switch the incremental schema activation from the global function to the proposed application level setting. This is definitely a better approach as it would not require to rebuild the code if someone needs to change the setting.

I saw there is an class NativeAppStorage in core-backend which allows to specify settings per application.
In core-frontend, we have a similar concept with a Storage on the NativeApp class.
Just to make sure we're going the right direction here, this are the classes we're suppose to use for this cases or are there any other apis I haven't seen yet?

Also, we follow Grigas comment and always activate the incremental schema loading by default, if a client runs into issues on their side, it can then use the app config to opt-out to get the previous behavior.

@pmconne
Copy link
Member

pmconne commented Oct 17, 2025

I saw there is an class NativeAppStorage in core-backend which allows to specify settings per application. In core-frontend, we have a similar concept with a Storage on the NativeApp class. Just to make sure we're going the right direction here, this are the classes we're suppose to use for this cases or are there any other apis I haven't seen yet?

Also, we follow Grigas comment and always activate the incremental schema loading by default, if a client runs into issues on their side, it can then use the app config to opt-out to get the previous behavior.

Typically, apps configure the behavior of the core packages through IModelAppOptions (frontend) and/or IModelHostOptions (backend). The app can decide how to choose the configuration (e.g., using a feature flagging service, hard-coding features they always/never want enabled, whatever). The NativeAppStorage APIs you mention aren't really relevant here.

@StefanApfel-Bentley
Copy link
Contributor Author

StefanApfel-Bentley commented Oct 17, 2025

Typically, apps configure the behavior of the core packages through IModelAppOptions (frontend) and/or IModelHostOptions (backend). The app can decide how to choose the configuration (e.g., using a feature flagging service, hard-coding features they always/never want enabled, whatever). The NativeAppStorage APIs you mention aren't really relevant here.

But that would again a code only approach, which means, clients have to specify their options explicitly on startup.
They can (or should) take the setting from an env (for example), but that would mean we'd have to add the settings flag to the public api (the startup options in this case). Which is not ideal, as we originally planned to "hide" that flag. using the stores directly could help archive that, but it's far less intuitive than through the options.

On the other hand, if I look at the options in the IModelAppOptions or IModelHostOptions, the public settings rather allow to set whole components than having flags that control the behavior of certain features inside the internals IModels have to work as they work, which is why I feel currently hesitant to expose that as a public option.

@pmconne
Copy link
Member

pmconne commented Oct 17, 2025

f I look at the options in the IModelAppOptions or IModelHostOptions, the public settings rather allow to set whole components than having flags that control the behavior of certain features inside the internals IModels have to work as they work, which is why I feel currently hesitant to expose that as a public option.

I'm afraid I don't follow your misgivings. For example, TileAdmin configuration provided at startup is exactly a set of "flags that control the behavior of certain features inside the internals", provided for seemingly the same purposes as your new feature (either opting in to a new experimental feature, or opting out of such a feature if it causes problems).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants