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

Accessing nested properties in flat style definitions #15505

Closed
WouterSpaak opened this issue Jan 23, 2024 · 8 comments · Fixed by #15813
Closed

Accessing nested properties in flat style definitions #15505

WouterSpaak opened this issue Jan 23, 2024 · 8 comments · Fixed by #15813

Comments

@WouterSpaak
Copy link

Is your feature request related to a problem? Please describe.

We're using OpenLayers extensively in our project. First of all: thank you for this amazing library!

At the moment we're exploring options where our users may be able to customize or define their own styles for layers. We're looking into defining all our available layers, as well as custom data, in a database, and sending flat styles over the wire from our API backend instead of hardcoding new Style(...) in code.

Things are looking promising, but we have quite a few layers with properties which are actually arbitrarily deeply nested objects. For instance:

const connection = new Feature();
connection.set('demand', '100KW');
connection.set('year_of_construction', '1927');
connection.set('address', {
  country: 'The Netherlands',
  province: 'Gelderland',
  municipality: 'Nijmegen',
  // etc
});

Say we want to use address.province in a filter expression, or a get expression. As far as I can tell, this is currently not supported, and only expressions where feature.get('key') which return a JS literal can be used.

Describe the solution you'd like

Hopefully I'm wrong and this is possible. If not, I've been diving into the code in src/ol/expression.js, and I propose a new nested expression. Given the previous address feature, something like:

const layer = new VectorLayer({
  style: {
    'text-value': ['get', ['nested', 'address', 'province']]
  }
});

Or a bit more formally:

['nested', key1, key2 ... keyN]: Traverses an object of type T where key1 extends keyof T, key2 extends keyof T[key1], keyN extends T[keyN-1] and returns the value at path N.

Op.Nested, when walked down the JS object path, should return a LiteralExpression (I think). This should also work out of the box in any other expressions or operators where a literal is expected.

Please let me know if this is something you may wish to pursue. I think, with a bit more time spent browsing through the code base in expression.js and cpu.js, I should be able to implement this and file a Pull Request.

@jahow
Copy link
Contributor

jahow commented Jan 23, 2024

Hi!

In the Maplibre style spec which is the main inspiration for the operators in OpenLayers expressions, the get operator actually allows this with a second argument: https://maplibre.org/maplibre-style-spec/expressions/#get

Note that the get operator in OL expressions takes a typeHint as 2nd param so this would also have to be handled. Type hints are string literals and not expressions so differentiating between an object and a type hint should be doable.

Would you be willing to provide a PR if that solution fits your needs? I'd be happy to provide guidance the best I can.

@tschaub
Copy link
Member

tschaub commented Jan 25, 2024

I agree it makes sense to support deeply nested feature property values. I'm not sure that the nested expression is really the best way to do this.

I think the most natural / guessable syntax would be for the get operator to take multiple arguments representing the nested path.

So for a feature with properties like

{some: {deeply: {nested: {value: 42}}}}

the expression

// my ideal syntax
['get', 'some', 'deeply', 'nested', 'value']

would evaluate to 42 in the context of the feature.

If instead we followed the Mapbox style convention and accepted an optional object as the second argument to the get operator, accessing the value above would look like this:

// following mapbox
['get', 'value', ['get', 'nested', ['get', 'deeply', ['get', 'some']]]]

Both of these approaches conflict with the current signature for get that accepts a type hint as the second argument. I think it is worth revisiting that decision (and changing it in a breaking release if we agree on something else). I feel like the type hints should be replaced with type assertions like the existing number and string operators. @jahow, are there cases where a type assertion could not be used instead of a type hint?

I'm not clear on what the proposed nested operator expression would evaluate to. And I'd like to have a bit more discussion on how to best handle nested property access before committing to something.

If we want to stick close to the Mapbox style spec, then accepting an object as the optional second argument to the get operator would make sense. And if we really needed to keep the existing type hint support, we could say the second argument must either be a string literal (type hint) or an expression that evaluates to an object (like another get expression). Implementing this (with or without the type hint support) would complicate the way the get operator currently works (which is to signal which feature properties are added to the evaluation context).

The simpler alternative to implement would be the ['get', 'some', 'deeply', 'nested', 'value'] syntax. This would also be my preference for being intuitive/guessable. I know it breaks from the Mapbox convention, but we already do in other ways.

@jahow
Copy link
Contributor

jahow commented Jan 25, 2024

Hmm, I still like the Mapbox approach quite a bit, it might be more verbose but it also makes things more explicit. This would also let your read from an array in an object using at and this sort of things. Wouldn't that be a bit awkward with just a list of string? I guess one could allow integers like so: ['get', 'array', 2, 'object', 'value'] but it feels quite implicit IMO.

About the type hint, once type assertions are available in the gpu compiler we can very likely get rid of them, yes!

@WouterSpaak
Copy link
Author

I agree with @tschaub that the nested proposal is a bit awkward, I figured it would be a way to minimise breaking changes to the API. It would overload get instead of changing the original call signature.

I really like the variadic API @tschaub suggests. It feels to me like very clean and intuitive API design. I'm not too keen on the verbose nature of ['get', 'parent', ['get', 'child', ['get', 'grandchild']], 'someTypeHint'] that the MapLibre spec implements, but it would be a bit easier to implement I guess, when Array.isArray(arguments[1]) we can traverse the JS object.

I'm not quite sure what the type hint argument does. Is there any documentation I could read? I'd be more than happy to help refactor something to get rid of it!

@jahow
Copy link
Contributor

jahow commented Jan 25, 2024

The type hint is there because the get operator has no clue what type of value it will output on its own. Other operators are usually able to infer their type by various means, but the get operator isn't and in some situations, having no informating about the output type will make parsing/compiling impossible. Because a user normally knows what type a feature attribute is, providing a type hint is an easy solution this.

@tschaub
Copy link
Member

tschaub commented Jan 25, 2024

The thing I'm not clear about with the type hint is where it is actually required. I understand how it works currently, but am uncertain where it is required. When parsing, we start with the top-most expression and we can know what output types are expected. For example, when parsing an expression for stroke-width, we expect a number. In the simple case of 'stroke-width': ['get', 'some-property'], we don't need a type hint. As we traverse the expressions, ideally we can narrow the types we expect. This works well if our operators are not polymorphic (for example, if + only accepts numbers and we have a separate operator like concat if we want to "add" strings together).

I know that the current implementation might need the type hints. But I'm still unclear if they are really "required" (if we change the current implementation or if we tighten up the syntax/operators).

@ahocevar
Copy link
Member

ahocevar commented May 6, 2024

The simpler alternative to implement would be the ['get', 'some', 'deeply', 'nested', 'value'] syntax. This would also be my preference for being intuitive/guessable. I know it breaks from the Mapbox convention, but we already do in other ways.

I'm strongly in favor of this simpler alternative and the resulting breaking change. If we do that, we also don't need an 'at' operator to access array properties. If we have properties like

{"answer": {"at": {"index": [17, 42]}}}

we could get the correct answer with

["get", "answer" , "at", "index", "1"]

@jahow Is this something you could live with for the WebGL implementation? If a type hint is needed, wouldn't

["number", ["get", "answer" , "at", "index", "1"]]

be a good way to do that? At least that's what Mapbox does.

@jahow
Copy link
Contributor

jahow commented May 6, 2024

Type assertions can definitely replace the type hint of the get operator. As for the simpler syntax mentioned here, looking at it now it just looks simple and functional, I can't find any drawback so +1 for me!

I know that the current implementation might need the type hints. But I'm still unclear if they are really "required" (if we change the current implementation or if we tighten up the syntax/operators).

I think at some point this ended up being necessary because a property could accept multiple types and just feeding it a "get" expression was not resolving to a single type. In general I think we cannot exclude this to happen, but if we rely on assertions then it's fine and we don't need the type hint anymore.

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

Successfully merging a pull request may close this issue.

4 participants