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

Same table names in different namespaces not supported #180

Open
jmillan opened this issue Apr 13, 2023 · 4 comments
Open

Same table names in different namespaces not supported #180

jmillan opened this issue Apr 13, 2023 · 4 comments

Comments

@jmillan
Copy link

jmillan commented Apr 13, 2023

As you can see in the following example, having tables with the same name in different namespaces is not supported. They are reduced to one (ie: Check for DumpResponse).

In our case we're using a quite wide schema and having namespaces make perfect sense.

.fbs schema:

union Body {
  FBS.Worker.DumpResponse,
  FBS.Worker.ResourceUsageResponse,
  FBS.WebRtcServer.DumpResponse,
  FBS.Router.DumpResponse,
  FBS.Transport.ProduceResponse,
  FBS.Transport.ConsumeResponse,
  FBS.Transport.RestartIceResponse,
  FBS.PlainTransport.ConnectResponse,
  FBS.PlainTransport.DumpResponse,
  FBS.PlainTransport.GetStatsResponse,
  FBS.PipeTransport.ConnectResponse,
  FBS.PipeTransport.DumpResponse,
  FBS.PipeTransport.GetStatsResponse,
  FBS.DirectTransport.DumpResponse,
  FBS.DirectTransport.GetStatsResponse,
  FBS.WebRtcTransport.ConnectResponse,
  FBS.WebRtcTransport.DumpResponse,
  FBS.WebRtcTransport.GetStatsResponse,
  FBS.Producer.DumpResponse,
  FBS.Producer.GetStatsResponse,
  FBS.Consumer.DumpResponse,
  FBS.Consumer.GetStatsResponse,
  FBS.Consumer.SetPreferredLayersResponse,
  FBS.Consumer.SetPriorityResponse,
  FBS.DataProducer.DumpResponse,
  FBS.DataProducer.GetStatsResponse,
  FBS.DataConsumer.GetBufferedAmountResponse,
  FBS.DataConsumer.DumpResponse,
  FBS.DataConsumer.GetStatsResponse,
}

The following rust code is generated:

            pub enum Body {
                DumpResponse(::planus::alloc::boxed::Box<super::data_consumer::DumpResponse>),
                ResourceUsageResponse(
                    ::planus::alloc::boxed::Box<super::worker::ResourceUsageResponse>,
                ),
                ProduceResponse(::planus::alloc::boxed::Box<super::transport::ProduceResponse>),
                ConsumeResponse(::planus::alloc::boxed::Box<super::transport::ConsumeResponse>),
                RestartIceResponse(
                    ::planus::alloc::boxed::Box<super::transport::RestartIceResponse>,
                ),
                ConnectResponse(
                    ::planus::alloc::boxed::Box<super::web_rtc_transport::ConnectResponse>,
                ),
                GetStatsResponse(
                    ::planus::alloc::boxed::Box<super::data_consumer::GetStatsResponse>,
                ),
                SetPreferredLayersResponse(
                    ::planus::alloc::boxed::Box<super::consumer::SetPreferredLayersResponse>,
                ),
                SetPriorityResponse(
                    ::planus::alloc::boxed::Box<super::consumer::SetPriorityResponse>,
                ),
                GetBufferedAmountResponse(
                    ::planus::alloc::boxed::Box<super::data_consumer::GetBufferedAmountResponse>,
                ),
            }

This is the generated code for c++:

enum class Body : uint8_t {
  NONE = 0,
  FBS_Worker_DumpResponse = 1,
  FBS_Worker_ResourceUsageResponse = 2,
  FBS_WebRtcServer_DumpResponse = 3,
  FBS_Router_DumpResponse = 4,
  FBS_Transport_ProduceResponse = 5,
  FBS_Transport_ConsumeResponse = 6,
  FBS_Transport_RestartIceResponse = 7,
  FBS_PlainTransport_ConnectResponse = 8,
  FBS_PlainTransport_DumpResponse = 9,
  FBS_PlainTransport_GetStatsResponse = 10,
  FBS_PipeTransport_ConnectResponse = 11,
  FBS_PipeTransport_DumpResponse = 12,
  FBS_PipeTransport_GetStatsResponse = 13,
  FBS_DirectTransport_DumpResponse = 14,
  FBS_DirectTransport_GetStatsResponse = 15,
  FBS_WebRtcTransport_ConnectResponse = 16,
  FBS_WebRtcTransport_DumpResponse = 17,
  FBS_WebRtcTransport_GetStatsResponse = 18,
  FBS_Producer_DumpResponse = 19,
  FBS_Producer_GetStatsResponse = 20,
  FBS_Consumer_DumpResponse = 21,
  FBS_Consumer_GetStatsResponse = 22,
  FBS_Consumer_SetPreferredLayersResponse = 23,
  FBS_Consumer_SetPriorityResponse = 24,
  FBS_DataProducer_DumpResponse = 25,
  FBS_DataProducer_GetStatsResponse = 26,
  FBS_DataConsumer_GetBufferedAmountResponse = 27,
  FBS_DataConsumer_DumpResponse = 28,
  FBS_DataConsumer_GetStatsResponse = 29,
  MIN = NONE,
  MAX = FBS_DataConsumer_GetStatsResponse
};
@TethysSvensson
Copy link
Collaborator

This is definitely a bug we want to fix, though I'm not quite sure how we want to fix it.

In the meantime you should be able to work around the problem by manually naming the variants. See this test for an example of how to use this syntax.

@TethysSvensson
Copy link
Collaborator

My current thoughts on how to fix it:

The current naming scheme doesn't use the namespace at all. As I see it, we have the following options:

  • Option 1: Always use the namespaces in variant names.
  • Option 1a: Always use the namespaces in variant names when there are variants from different namespaces.
  • Option 2: Use namespaces in variant names when ambiguous.
  • Option 3: Output an error on ambiguity.
  • Option 3a: Output an error on ambiguity, but also add a custom attribute to enable using namespace in variant names.

Currently I think option 1 and 1a are a bad ideas, as it would make the common experience much worse.

I am currently split between options 2, 3 and 3a.

I think I am leaning towards 3, since I currently think that naming the variants is the currently feel that naming the variants is a good enough work-around. However I am definitely willing to be convinced otherwise.

In any case, I think it's a good idea to implement option 3 immediately, since we would need the code to detect ambiguity for 2 and 3a as well.

@jmillan
Copy link
Author

jmillan commented Apr 14, 2023

I personally think that 3a is a good solution:

  1. If there are namespaces but no ambiguity then the current implementation perfectly works and variant naming is kept minimal for better experience.
  2. If there are namespaces and there is ambiguity, the generated error allows you to:
    2.1. Modify the schema to fix the ambiguity.
    2.2. Add a custom attribute to enable using namespace in variant names.

2.1 fits new, small, or not yet in-use schemas.
2.2 fits schemas that are more complex to modify (or which modification would make it less maintainable) and those cases where the schemas are already in use on other languages around a service.

jmillan added a commit to versatica/mediasoup that referenced this issue Apr 14, 2023
Workaround until planus-org/planus#180 is
fixed.

This change keeps compatibility with C++ and TS.
@jmillan
Copy link
Author

jmillan commented Apr 14, 2023

In the meantime you should be able to work around the problem by manually naming the variants

Thanks for this suggestion @TethysSvensson 👍. It helps in the meantime.

jmillan added a commit to versatica/mediasoup that referenced this issue Sep 15, 2023
Use aliases for Message, Request and Notification bodies.

Needed for Rust[*], plus the resulting type names are much more
convenient, which come with the redundant 'FBS_' prefix.

[*] (planus-org/planus#180).
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

No branches or pull requests

2 participants