Add util methods to parse proto files with dependencies #3834

franz1981 · 2023-10-17T20:05:35Z

This is to open a discussion about a new public API to parse a file and its dependencies.

In the current state I don't like (at all) the other public API I've exposed for #3819

ie

        Descriptors.FileDescriptor mainProtoFd = FileDescriptorUtils.parseProtoFileWithDependencies(mainProto,
                mainProtoFileName,
                Map.of("mypackage0/producerId.proto", importedProto1, "mypackage2/version.proto", importedProto2));

apicurio-bot · 2023-10-17T20:05:40Z

Thank you for creating a pull request!

Pinging @andreaTP to respond or triage.

Caching of artifacts with no version (e.g. latest) did not work because reindex would use the new key that has the artifact version that was found in the lookup. This code changes the default behavior to index both the artifact with its version and the latest/null version. It also exposes a configuration property (`apicurio.registry.cache-latest`) where this behavior can be disabled for use cases where caching of latest is not desired. See Apicurio#3824 for details.

...-utilities/src/main/java/io/apicurio/registry/utils/protobuf/schema/FileDescriptorUtils.java

franz1981 · 2023-10-18T10:04:05Z

@carlesarnal I've missed few tests (on purpose) because I need first to understand which parts to change/improve (and maybe deduplicate, cause i've introduced a couple of wrappers for concept that are maybe already defined elsewhere)

Re the comment on the APi I would use in Hyperfoil at #3834 (comment), I've replaced it with something as simple as passing files, which is instead the parse method version used by Apicurio.

franz1981 · 2023-10-18T12:26:24Z

This PR doesn't just include the change to enable parsing protos + deps but it add a fix too to the ability to link protos which belong to different packages.
let me know if i need to split this in 2s

…a updates (#3839) * feat(schema-cache): ERCache.configureFaultTolerantRefresh #3807 (#3823) Adds the notion of a fault tolerance in refresh for production environments where it's better to use a stale cache value than die when a cache entry refresh fails. See issue #3807 for details. Co-authored-by: Devon Berry <devon.berry@riotgames.com> * fix(schema-resolver): caching of latest artifacts #3834 Caching of artifacts with no version (e.g. latest) did not work because reindex would use the new key that has the artifact version that was found in the lookup. This code changes the default behavior to index both the artifact with its version and the latest/null version. It also exposes a configuration property (`apicurio.registry.cache-latest`) where this behavior can be disabled for use cases where caching of latest is not desired. See #3824 for details. --------- Co-authored-by: Devon Berry <devon.berry@riotgames.com>

franz1981 · 2023-10-20T14:49:27Z

@carlesarnal

I've just noticed that loading the schema is keep on loading imports which are not used and it seems is due to

    private static FileSystem getFileSystem() throws IOException {
        final FakeFileSystem inMemoryFileSystem = new FakeFileSystem();
        inMemoryFileSystem.setWorkingDirectory(okio.Path.get("/"));
        inMemoryFileSystem.setAllowSymlinks(true);

        final ClassLoader classLoader = ProtobufSchemaLoader.class.getClassLoader();

        createDirectory(GOOGLE_API_PATH.split("/"), inMemoryFileSystem);
        loadProtoFiles(inMemoryFileSystem, classLoader, GOOGLE_API_PROTOS, GOOGLE_API_PATH);

        createDirectory(GOOGLE_WELLKNOWN_PATH.split("/"), inMemoryFileSystem);
        loadProtoFiles(inMemoryFileSystem, classLoader, GOOGLE_WELLKNOWN_PROTOS, GOOGLE_WELLKNOWN_PATH);

        createDirectory(METADATA_PATH.split("/"), inMemoryFileSystem);
        loadProtoFiles(inMemoryFileSystem, classLoader, Collections.singleton(METADATA_PROTO), METADATA_PATH);

        createDirectory(DECIMAL_PATH.split("/"), inMemoryFileSystem);
        loadProtoFiles(inMemoryFileSystem, classLoader, Collections.singleton(DECIMAL_PROTO), DECIMAL_PATH);

        return inMemoryFileSystem;
    }

which is placing dependencies which maybe won't be required to resolve the specific proto file (nor its dependencies).

This could be done in a follow-up PR but ideally we would like to re-use the transitive resolution of dependencies mechanism of this PR
to filter the effectively used types in the previous java snipped. detecting which ones are really necessary to resolve a given proto file (which I suppose is part of the imports section on it - or in its dependencies); given that each proto is solved by re-creating a new file system, is still not optimal (and this is another follow-up PR, actually), but it would reduce the files we place into it which will be loaded by https://github.com/square/wire/blob/d58f02b4072403a16ff891e26ec8e926869e0444/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/CommonSchemaLoader.kt#L120.

franz1981 · 2023-10-20T15:42:46Z

In addition I've checked how ProtobugDirectoryParser was resolving the dependencies of the failed test (now fixed ie autoRegisterProtoWithReferences)

Before this PR:

PARSE mode.proto schema deps = [] dependencies []
PARSE table_notification_type.proto schema deps = [mode.proto] dependencies [mode.proto]
PARSE table_info.proto schema deps = [mode.proto, table_notification_type.proto] dependencies [mode.proto, table_notification_type.proto]
PARSE table_notification.proto schema deps = [table_info.proto, mode.proto, table_notification_type.proto] dependencies [table_info.proto, mode.proto, table_notification_type.proto]

With this PR:

PARSE mode.proto schema deps = [] dependencies = []
PARSE table_info.proto schema deps = [mode.proto] dependencies = [mode.proto]
PARSE table_notification_type.proto schema deps = [] dependencies = []
PARSE table_notification.proto schema deps = [table_notification_type.proto, mode.proto, table_info.proto] dependencies = [table_info.proto, table_notification_type.proto]

And I've noticed that before this PR we always add ALL the base dependencies Fds, while this PR strip it out in order to use just the required one (which is a win).

carlesarnal · 2023-10-23T07:55:45Z

This PR doesn't just include the change to enable parsing protos + deps but it add a fix too to the ability to link protos which belong to different packages. let me know if i need to split this in 2s

No need to split, it's fine as it is.

carlesarnal · 2023-10-23T07:59:02Z

...utilities/src/main/java/io/apicurio/registry/utils/protobuf/schema/ProtobufSchemaLoader.java

@@ -93,6 +93,10 @@ private static void loadProtoFiles(FakeFileSystem inMemoryFileSystem, ClassLoade
        for (String proto : protos) {
            //Loads the proto file resource files.
            final InputStream inputStream = classLoader.getResourceAsStream(protoPath + proto);
+            // TODO tmp fix to run tests


Just remember to remove this :)

carlesarnal · 2023-10-23T08:04:55Z

@carlesarnal

I've just noticed that loading the schema is keep on loading imports which are not used and it seems is due to
    private static FileSystem getFileSystem() throws IOException {
        final FakeFileSystem inMemoryFileSystem = new FakeFileSystem();
        inMemoryFileSystem.setWorkingDirectory(okio.Path.get("/"));
        inMemoryFileSystem.setAllowSymlinks(true);

        final ClassLoader classLoader = ProtobufSchemaLoader.class.getClassLoader();

        createDirectory(GOOGLE_API_PATH.split("/"), inMemoryFileSystem);
        loadProtoFiles(inMemoryFileSystem, classLoader, GOOGLE_API_PROTOS, GOOGLE_API_PATH);

        createDirectory(GOOGLE_WELLKNOWN_PATH.split("/"), inMemoryFileSystem);
        loadProtoFiles(inMemoryFileSystem, classLoader, GOOGLE_WELLKNOWN_PROTOS, GOOGLE_WELLKNOWN_PATH);

        createDirectory(METADATA_PATH.split("/"), inMemoryFileSystem);
        loadProtoFiles(inMemoryFileSystem, classLoader, Collections.singleton(METADATA_PROTO), METADATA_PATH);

        createDirectory(DECIMAL_PATH.split("/"), inMemoryFileSystem);
        loadProtoFiles(inMemoryFileSystem, classLoader, Collections.singleton(DECIMAL_PROTO), DECIMAL_PATH);

        return inMemoryFileSystem;
    }
which is placing dependencies which maybe won't be required to resolve the specific proto file (nor its dependencies).

This could be done in a follow-up PR but ideally we would like to re-use the transitive resolution of dependencies mechanism of this PR to filter the effectively used types in the previous java snipped. detecting which ones are really necessary to resolve a given proto file (which I suppose is part of the imports section on it - or in its dependencies); given that each proto is solved by re-creating a new file system, is still not optimal (and this is another follow-up PR, actually), but it would reduce the files we place into it which will be loaded by https://github.com/square/wire/blob/d58f02b4072403a16ff891e26ec8e926869e0444/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/CommonSchemaLoader.kt#L120.

Yes, this was done just to be sure we have all the well-known types available when resolving the main proto file in case any of them are being used. Obviously resolving the actually used ones and just adding them is way better than the current solution.

carlesarnal · 2023-10-23T08:06:25Z

In addition I've checked how ProtobugDirectoryParser was resolving the dependencies of the failed test (now fixed ie autoRegisterProtoWithReferences)

Before this PR:

PARSE mode.proto schema deps = [] dependencies []
PARSE table_notification_type.proto schema deps = [mode.proto] dependencies [mode.proto]
PARSE table_info.proto schema deps = [mode.proto, table_notification_type.proto] dependencies [mode.proto, table_notification_type.proto]
PARSE table_notification.proto schema deps = [table_info.proto, mode.proto, table_notification_type.proto] dependencies [table_info.proto, mode.proto, table_notification_type.proto]

With this PR:

PARSE mode.proto schema deps = [] dependencies = []
PARSE table_info.proto schema deps = [mode.proto] dependencies = [mode.proto]
PARSE table_notification_type.proto schema deps = [] dependencies = []
PARSE table_notification.proto schema deps = [table_notification_type.proto, mode.proto, table_info.proto] dependencies = [table_info.proto, table_notification_type.proto]

And I've noticed that before this PR we always add ALL the base dependencies Fds, while this PR strip it out in order to use just the required one (which is a win).

Same as above, the old parser was just making anything "parsable" available. This is obviously a win (just as you said :))

Caching of artifacts with no version (e.g. latest) did not work because reindex would use the new key that has the artifact version that was found in the lookup. This code changes the default behavior to index both the artifact with its version and the latest/null version. It also exposes a configuration property (`apicurio.registry.cache-latest`) where this behavior can be disabled for use cases where caching of latest is not desired. See #3824 for details. Co-authored-by: Devon Berry <devon.berry@riotgames.com>

apicurio-bot bot added the triage/needs-triage label Oct 17, 2023

franz1981 commented Oct 18, 2023

View reviewed changes

...-utilities/src/main/java/io/apicurio/registry/utils/protobuf/schema/FileDescriptorUtils.java Outdated Show resolved Hide resolved

franz1981 commented Oct 18, 2023

View reviewed changes

...-utilities/src/main/java/io/apicurio/registry/utils/protobuf/schema/FileDescriptorUtils.java Show resolved Hide resolved

franz1981 force-pushed the batch_dep_solver branch from 6fd8906 to 61efb2a Compare October 18, 2023 09:55

franz1981 marked this pull request as ready for review October 18, 2023 10:02

franz1981 force-pushed the batch_dep_solver branch from 61efb2a to c7c5fef Compare October 18, 2023 10:04

franz1981 force-pushed the batch_dep_solver branch 2 times, most recently from b23b57e to 50b7217 Compare October 18, 2023 12:34

franz1981 force-pushed the batch_dep_solver branch from 64203b7 to 10615e3 Compare October 20, 2023 14:17

franz1981 force-pushed the batch_dep_solver branch from 10615e3 to cd32a4f Compare October 21, 2023 11:27

carlesarnal reviewed Oct 23, 2023

View reviewed changes

franz1981 force-pushed the batch_dep_solver branch 2 times, most recently from 4f0b335 to 63545d8 Compare October 23, 2023 10:42

Add util methods to parse proto files with dependencies

d565880

franz1981 force-pushed the batch_dep_solver branch from 397e727 to d565880 Compare October 23, 2023 10:44

Merge branch 'main' into batch_dep_solver

652a8b7

carlesarnal merged commit 9c54710 into Apicurio:main Oct 25, 2023
20 checks passed

apicurio-bot bot removed the triage/needs-triage label Oct 25, 2023

carlesarnal added the backport-2.x Backport fix to Registry v2 label Nov 3, 2023

carlesarnal self-assigned this Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add util methods to parse proto files with dependencies #3834

Add util methods to parse proto files with dependencies #3834

franz1981 commented Oct 17, 2023 •

edited

Loading

apicurio-bot bot commented Oct 17, 2023

franz1981 commented Oct 18, 2023

franz1981 commented Oct 18, 2023 •

edited

Loading

franz1981 commented Oct 20, 2023 •

edited

Loading

franz1981 commented Oct 20, 2023

carlesarnal commented Oct 23, 2023

carlesarnal Oct 23, 2023

carlesarnal commented Oct 23, 2023

carlesarnal commented Oct 23, 2023

Add util methods to parse proto files with dependencies #3834

Add util methods to parse proto files with dependencies #3834

Conversation

franz1981 commented Oct 17, 2023 • edited Loading

apicurio-bot bot commented Oct 17, 2023

franz1981 commented Oct 18, 2023

franz1981 commented Oct 18, 2023 • edited Loading

franz1981 commented Oct 20, 2023 • edited Loading

franz1981 commented Oct 20, 2023

carlesarnal commented Oct 23, 2023

carlesarnal Oct 23, 2023

Choose a reason for hiding this comment

carlesarnal commented Oct 23, 2023

carlesarnal commented Oct 23, 2023

franz1981 commented Oct 17, 2023 •

edited

Loading

franz1981 commented Oct 18, 2023 •

edited

Loading

franz1981 commented Oct 20, 2023 •

edited

Loading