Skip to content

Conversation

DimaVilda
Copy link
Contributor

@DimaVilda DimaVilda commented Dec 8, 2024

issue link:
Fixes #650

  • Breaking change? (if so, please describe the impact and migration path for existing application instances)

What changes did you make?

  • Fixed serialization of Protobuf messages containing Any type fields
  • Added TypeRegistry support in ProtobufFileSerde to properly handle Any type resolution
  • Added test cases to verify handling of Any type messages

Is there anything you'd like reviewers to focus on?

  • Implementation of TypeRegistry in the serializer method
  • Integration test to check this case

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • No need to
  • Manually (please, describe, if necessary) - bug was reproduced on self made project using steps from issue 650, checked error stack trace to see what is wrong
  • Unit checks
  • Integration checks
  • Covered by existing automation

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. ENVIRONMENT VARIABLES)
  • My changes generate no new warnings (e.g. Sonar is happy)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

Check out Contributing and Code of Conduct

A picture of a cute animal (not mandatory but encouraged)

@DimaVilda DimaVilda requested a review from a team as a code owner December 8, 2024 20:48
@kapybro kapybro bot added status/triage Issues pending maintainers triage status/triage/manual Manual triage in progress status/triage/completed Automatic triage completed and removed status/triage Issues pending maintainers triage labels Dec 8, 2024
@DimaVilda
Copy link
Contributor Author

DimaVilda commented Dec 8, 2024

hey team
one more thing to notice!
ProtobufFileSerdeTest and MessagesServiceTest tests don't work on Windows environment. error is:
' ProtoFile not found for import 'language/language.proto''

Why?
Because in ProtoSchemaLoader you call

  Map<String, ProtoFile> filesByLocations = new HashMap<>();
  filesByLocations.putAll(knownTypes);
  filesByLocations.putAll(loadFilesWithLocations());

and the problem is in 'loadFilesWithLocations' and in

private Loader createFilesLoader(Map<String, ProtoFile> files) {
  return new Loader() {
    @Override
    public @NotNull ProtoFile load(@NotNull String path) {
      return Preconditions.checkNotNull(files.get(path), "ProtoFile not found for import '%s'", path);
    }

methods because in Windows paths use backslashes () but proto imports use forward slashes (/), so better would be create a ticket to normalize paths on both OS and be able to run project. I will create it and provide you a fix for it with a few additional test cases.
Feel free to ask me questions

@Haarolean
Copy link
Member

hey team one more thing to notice! ProtobufFileSerdeTest and MessagesServiceTest tests don't work on Windows environment. error is: ' ProtoFile not found for import 'language/language.proto''

Why? Because in ProtoSchemaLoader you call

  Map<String, ProtoFile> filesByLocations = new HashMap<>();
  filesByLocations.putAll(knownTypes);
  filesByLocations.putAll(loadFilesWithLocations());

and the problem is in 'loadFilesWithLocations' and in

private Loader createFilesLoader(Map<String, ProtoFile> files) {
  return new Loader() {
    @Override
    public @NotNull ProtoFile load(@NotNull String path) {
      return Preconditions.checkNotNull(files.get(path), "ProtoFile not found for import '%s'", path);
    }

methods because in Windows paths use backslashes () but proto imports use forward slashes (/), so better would be create a ticket to normalize paths on both OS and be able to run project. I will create it and provide you a fix for it with a few additional test cases. Feel free to ask me questions

@DimaVilda yeah aware of it, been discussed here: #261. If you can fix them without altering production code, I'd be happy to accept such changes.

@Haarolean Haarolean requested a review from iliax December 9, 2024 16:44
@Haarolean Haarolean added scope/backend Related to backend changes type/bug Something isn't working area/serde Serialization & Deserialization (plugins) and removed status/triage/manual Manual triage in progress labels Dec 9, 2024
@Haarolean Haarolean changed the title Issues/650: Fix 500 error on protobuf Any type BE: Fix HTTP 500on protobuf Any type Dec 9, 2024
@Haarolean Haarolean changed the title BE: Fix HTTP 500on protobuf Any type BE: Fix HTTP 500 on protobuf Any type Dec 9, 2024
@DmytroVilda
Copy link

hey team one more thing to notice! ProtobufFileSerdeTest and MessagesServiceTest tests don't work on Windows environment. error is: ' ProtoFile not found for import 'language/language.proto''
Why? Because in ProtoSchemaLoader you call

  Map<String, ProtoFile> filesByLocations = new HashMap<>();
  filesByLocations.putAll(knownTypes);
  filesByLocations.putAll(loadFilesWithLocations());

and the problem is in 'loadFilesWithLocations' and in

private Loader createFilesLoader(Map<String, ProtoFile> files) {
  return new Loader() {
    @Override
    public @NotNull ProtoFile load(@NotNull String path) {
      return Preconditions.checkNotNull(files.get(path), "ProtoFile not found for import '%s'", path);
    }

methods because in Windows paths use backslashes () but proto imports use forward slashes (/), so better would be create a ticket to normalize paths on both OS and be able to run project. I will create it and provide you a fix for it with a few additional test cases. Feel free to ask me questions

@DimaVilda yeah aware of it, been discussed here: #261. If you can fix them without altering production code, I'd be happy to accept such changes.

@Haarolean , wdym without altering production code? There is incorrect backslashes processing in loadFilesWithLocations() so how can we fix it without touching it ? Or did I misunderstand your commet ?

@Haarolean Haarolean merged commit d5c976e into kafbat:main Dec 20, 2024
11 of 13 checks passed
@Haarolean
Copy link
Member

@DimaVilda thanks for your first contribution to kafbat UI!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/serde Serialization & Deserialization (plugins) scope/backend Related to backend changes status/triage/completed Automatic triage completed type/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serde: HTTP 500 when proto files contain Any type
4 participants