Make functionNullability property consistent in ResolvedFunction

[weiatwork]

The mismatch of "nullability" and "functionNullability" could cause
failure of JSON deserialization for ResolvedFunction

(This PR is to replace #13402 which had merge issue)

The symptom is query that involves function usages fails with error message like:
java.lang.IllegalArgumentException: Invalid JSON bytes for [simple type, class io.trino.metadata.ResolvedFunction]

Root cause is #10183, and specifically 2c1ebd6

The problem is exposed starting from #11217

Repro steps:

1. Create a file-based access control policy for a table with row filter or column mask that uses a function "filter": "name LIKE 'A%'"
2. Run a query against the table

Description

Is this change a fix, improvement, new feature, refactoring, or other?
fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)
core query engine
How would you describe this change to a non-technical end user or system administrator?
n/a

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[findepi]

The mismatch of "nullability" and "functionNullability" could cause
failure of JSON deserialization for ResolvedFunction

When running locally from IDE, under debugger I verified that during JSON serialization the functionNullability field gets serialized to nullability JSON attribute. The change of @JsonProperty("nullability") in the constructor gets reflected in the serialization.
I didn't know that JSON serialization can be affected by the @JsonProperty annotations on the @JsonCreator constructor.

If this is a reliable mechanism, that means we don't have a bug (but the change is still a good change).
If this mechanism shouldn't be relied on, we probably have a bug.

@electrum you probably know how and when this works

cc @dain

[weiatwork]

@findepi You're right. Or if we want to keep the constructor annotation unchanged @JsonProperty("nullability") FunctionNullability functionNullability, then we will have to update the getter's annotation to

@JsonProperty("nullability")
    public FunctionNullability getFunctionNullability()

The point is the name of JsonProperty need to be consistent.

[findepi]

The point is the name of JsonProperty need to be consistent.

that's what i believe is true

then we will have to update the getter's annotation to

i'd like to understand whether it's "have to" or "should" or "it would be nice to".
That's why I tried to reproduce the failure and discovered the serialization behavior being dependent on the deserialization annotations (#13777 (comment))
@electrum @dain @martint do you know when this works and when it doesn't?

[martint]

That's why I tried to reproduce the failure and discovered the serialization behavior being dependent on the deserialization annotations

It's not supposed to work that way. AFAICT, that's a behavior change in recent versions of Jackson -- not sure if intentional or a bug, but certainly a backward incompatible change, and at a minimum, very surprising. For example, if you run the following test within Trino:

public class Container
{
    private int value;

    @JsonCreator
    public Container(@JsonProperty("foo") int value)
    {
        this.value = value;
    }

    @JsonProperty
    public int getValue()
    {
        return value;
    }

    public static void main(String[] args) {}
    {
        System.out.println(jsonCodec(Container.class).toJson(new Container(1)));
    }
}

it produces:

{
  "foo" : 1
}

but it you run it within Airlift, it produces:

{
  "value" : 1
}

My guess is that it's linking the getter name to the argument name and inferring the property name from that annotation (and ignoring the fact that the getter already has an annotation and the rules that govern it). This is supported by the fact that if you change the variable name to something else, it starts producing the variant with value within Trino.

If intentional, this is a very surprising behavior in Jackson. It's now not possible to look at a getter declaration and make inferences about how the property will be named just from the signature of the method and any annotation on it:

@JsonProperty
public int getValue() { ... } 

[weiatwork]

@findepi Can you please take another look? I'm still experiencing this issue, as of Trino 412. It would be great if we can incorporate this PR since it's harmless and makes the logic more consistent

[findepi]

@weiatwork it would definitely help if there was a regression test associated with this change

[weiatwork]

Thanks @findepi for your response. I initially opened this PR because I ran into the problem with the presence of a file-based access control.

Just to clarify, now I can reproduce the problem with the vanilla Trino 412 code, without any customization or addition. A simple SELECT query submitted to server would trigger it. However, the UT TestResolvedFunction still passes.

[findepi]

@weiatwork i am glad you are able to reproduce the problem!
This clearly identifies an important gap in our testing, which probably is even more important that a particular problem with resolved functions.

However, the UT TestResolvedFunction still passes.

can you perhaps try reproducing the problem as a product test?

[weiatwork]

Thanks @findepi. Just FYI, from my side, this can be reproduced by simply launching Trino server (from within IDE), and run a SELECT query from CLI. I doubt this is a common issue for everyone else because it would have been reported already if that's the case. It could be due to some Jackson lib dependency from my local setup that somehow was being picky(?).

Anyway, the bottom line is the annotation should be consistent between the constructor and the getters. Otherwise, the potential issue is just there waiting to be triggered. If you think this change is unnecessary, feel free to close the PR.

[findepi]

My understanding is that

• we have annotations inconsistency
• the problem is masked by jackson somehow and Trino is currently not affected
• @weiatwork is able to reproduce the problem on their fork due to some other, unknown modifications (perhaps different jackson version)

if indeed so, we should still merge the change, but the regression tes is not possible