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

Make our Converter Interface Converter<S,T> #5

Open
radcortez opened this issue Mar 30, 2020 · 21 comments · May be fixed by #23
Open

Make our Converter Interface Converter<S,T> #5

radcortez opened this issue Mar 30, 2020 · 21 comments · May be fixed by #23

Comments

@radcortez
Copy link
Member

radcortez commented Mar 30, 2020

Right now, our Converter interface is generic type for the conversion result, but the conversion original value is fixed to a String.

It might be interesting for our top level conversion to support a generic type on the original value as well, like:

public interface Converter<S, D> extends Serializable {
    D convert(S value);
}

The motivations behind this is to better support requirements in MP JWT. Original issue here:
eclipse/microprofile-jwt-auth#100

The source object from where values are retrieved from is a Json structure (the JWT). Values are represented as the Json counterparts (JsonObject, JsonNumber, etc), so in fact, our conversion needs to call .toString on these and perform additional operations back and forward for the final conversion to take place.

With String conversion

Object claim = jsonWebToken.getClaim(claimName);
String value = claim.toString(); // this does Integer.toString(num);
Integer integer = converters.convertValue(value, Integer.class); // and we convert from a string to int again.

With Source and Target

Object claim = jsonWebToken.getClaim(claimName);
Integer integer = converters.convertValue(value, Integer.class);

JsonNumberToIntegerConverter implements Converter<JsonNumber,Integer> {
  Integer convert(JsonNumber value) {
   return value.intValue();
  }
}
@radcortez
Copy link
Member Author

@dmlloyd What do you think of this?

@dmlloyd
Copy link

dmlloyd commented Mar 31, 2020

Are you expecting that converters get automatically chained? If I've registered a Converter<Foo,Bar> and a Converter<Bar,Baz>, do I automatically have a Converter<Foo,Baz>? Why or why not?

@radcortez
Copy link
Member Author

I'm not so sure if we should go that route yet. I fear that if we go that route, we may become another full object mapper library like MapStruct, ModelMapper, Dozer, etc.

We may reach the conclusion that we will need chaining, but I think not at this stage. I think that it is acceptable that if you require conversion of composed objects, it should be handled manually calling the appropriate converts in the entry point converter.

What do you think?

@dmlloyd
Copy link

dmlloyd commented Mar 31, 2020

I'm not sure I have an opinion yet, just questions. :-)

Another case that I wonder about is collection conversion. How would it work? Today since converters are one-dimensional, we can easily define every possible conversion in terms of other converters (for example we can compose optional, list, and a scalar type all into one chain) in a very straightforward way. But what would be the correct composition approach here? Would we have a converter from a String to a List<String> and then a converter from List<String> to List<T>? How would other parametric types work?

One feature that SmallRye Config misses that I think would be powerful would be automatic composition. For example I should be able to register a global converter like this:

public final class MySetConverter<T> implements Converter<Set<T>> {
    private final Converter<T> delegate;
    public MySetConverter(Converter<T> delegate) {
        this.delegate = delegate;
    }
    public Set<T> convert(String input) {
        // do the conversion here using my custom Set impl
    }
}

Then the config framework examines the signature, notes I have a type var T and that the constructor has a corresponding parameter of type Converter (or Class), and registers the set implementation as a default for all sets, eliminating the need for special compositional factory methods like we have today (see smallrye/smallrye-config#40).

Is such a feature even possible when converters are two-dimensional? I'm not so sure.

@radcortez
Copy link
Member Author

radcortez commented Mar 31, 2020

Most likely Collections conversion will need to be handle differently. We need to only require the element type converter of the Collection to be implemented, and then we handle the rest.

This means that for these cases we only support Collection<S> to Collection<D> conversion. Not sure how useful would it be to support S to Collection<D> automatically, unless you supply the specific converter of course.

Then you can have specialised Converter types, for instance String like we have today, and keep the same behaviour.

I see no reason for smallrye/smallrye-config#40 wouldn't work for two-dimensional types. In theory, you should able to implement that. On practice, it may provide too much work :)

@dmlloyd
Copy link

dmlloyd commented Mar 31, 2020

I don't mean two dimensional arrays, I mean two dimensions of typing.

The canonical theoretical problem is like this: if I have a Converter<S, Foo> and a Converter<Bar, D>, and I want to convert a Bar to a Foo, which converter is used, and why?

@radcortez
Copy link
Member Author

Ops. Sorry. I meant types not arrays. Fixed.

That is an interesting question. I would probably lean to a Converter<S, Foo> for the sole reason that your result should be the most accurate one. Of course, you cannot guarantee that, but I think the Converter focus should always be in the the type being returned, because that is the one consumers are probably more interested in, or why would they need to convert it in the first place? :)

@dmlloyd
Copy link

dmlloyd commented Mar 31, 2020

Ops. Sorry. I meant types not arrays. Fixed.

That is an interesting question. I would probably lean to a Converter<S, Foo> for the sole reason that your result should be the most accurate one. Of course, you cannot guarantee that, but I think the Converter focus should always be in the the type being returned, because that is the one consumers are probably more interested in, or why would they need to convert it in the first place? :)

Sure, but that's a subjective argument, and thus the exact opposite argument could be made with equal validity. The fact is that both converters have equal validity from any objective measure, therefore whatever option is chosen will be wrong to some extent. And who wants to use wrong software?

But we don't need to give up all hope; every problem is solvable. But before we go into that, do we have other use cases beyond Config and JWT that need to be evaluated?

@radcortez
Copy link
Member Author

Sure. But we are here to fix the issues we took the wrong decision. I don't loose hope with you here :)

Right now, I've looked into this with the JWT perspective, because it was an obvious use for the library and even have open issues about supporting Converts like Config.

Maybe OpenTracing / OpenTelemetry could also benefit from these, so you could inject and convert to types directly from the trace context (but we will be dealing with String here).

@dmlloyd
Copy link

dmlloyd commented Mar 31, 2020

OK so we have two cases; in one case, the input is a String and in the other the input is some structured type of known complexity.

I would go a little farther and say that considering smallrye/smallrye-config#216 and other YAML-related issues, plus eclipse/microprofile-config#550, we might consider that configuration sources also would want to use structured types of known complexity rather than dealing just in raw strings all the time.

So for the sake of exploring this idea, let's assume that is the case, and let's assume we have some intermediate representation or representations that encapsulate all of the possible structured types.

Now things are interesting because we can have an InputConverter<T> which converts a T to the intermediate type(s), and an OutputConverter<T> which converts the intermediate type(s) to T.

From the perspective of config, configuration sources could produce the intermediate type(s) instead of String, and that solves the aforementioned issues quite cleanly. From the perspective of JWT, a converter from JSON to one or more of the intermediate type(s) would be available which could potentially solve that case as well.

The final feature of such a design is that the converter ambiguity problem is solved definitively. Every combination of input and output can be supported equally well because every converter type is one-dimensional.

But, what would the intermediate type(s) look like, and why? That's the next challenging question.

@dmlloyd
Copy link

dmlloyd commented Mar 31, 2020

It's also worth nothing that InputConverter<T> is not necessary.

@radcortez
Copy link
Member Author

And do you have any ideas of the intermediate types? Quite frankly, I couldn't think on any acceptable intermediate type.

Unless I didn't understand the concept fully, in practice you will have a 2 step conversion. One from source to the intermediary type and another one from the intermediary to destiny. Is that correct?

@dmlloyd
Copy link

dmlloyd commented Apr 1, 2020

And do you have any ideas of the intermediate types? Quite frankly, I couldn't think on any acceptable intermediate type.

Unless I didn't understand the concept fully, in practice you will have a 2 step conversion. One from source to the intermediary type and another one from the intermediary to destiny. Is that correct?

Right, if you do both halves of the concept - unless your intermediate type already includes your conversion source (or target). For example if the intermediate type (or one of the intermediate types) is a string, then you don't need an input converter to convert from string.

@radcortez
Copy link
Member Author

radcortez commented Apr 1, 2020

Ok, let go for a more complex case with the JsonObject. Lets say I want to convert a JsonObject into an Address.

So I have an InputConverter<JsonObject> and a OutputConvert<Address>, right? In this particular case it seems redundant to have an intermediary type between JsonObject and Address, since JsonObject is already a ready parsed representation to convert to something else.

Do we have specialised OutputConverters like JsonOutputConverter<Address> that uses JsonObject as the intermediary type? (maybe the name is misleading).

@dmlloyd
Copy link

dmlloyd commented Apr 1, 2020

On the one hand, it could be possible to have a structured intermediate type that can (like JSON or YAML for example) be an integer, FP number, string, boolean, or a list or map of any intermediate type. This solves the problem elegantly, in a way: the input could be JSON, or YAML, or a plain string; the value becomes the intermediate structured type; the output converter interprets the open ended type and produces an Address instance.

However, some context is lost. Perhaps interpretation of an Address differs between JSON and YAML due to different specifications. Perhaps there's more than one interpretation of Address even within JSON due to different standards specifying different formats.

So, this surrounding context would have to be carried forward somehow. The input converter would have to know where the information comes from in some standard way that can be interpreted by the output converter in a general way. This mechanism would have to be built to prevent the output converter from generating an object wrongly due to an unrecognized input context.

@radcortez
Copy link
Member Author

I'm not sure if this answers my questions completely.

My concern is that by introducing this intermediate type, we would be performing unneeded operations on structures that are already optimised for specific outputs.

Should we have a single InputConverter for an Address that can read data in any format? (JSON, YAML, String, etc). Or multiple ones? One for each? The intermediary type should be the same (only the context changes).

Maybe we can think about this as ConfigSource in the sense that you have multiple InputSources, each with priority and when you want to convert you either convert using the highest priority one, or you select the source you want to use?

@dmlloyd
Copy link

dmlloyd commented Apr 1, 2020

I'm not sure if this answers my questions completely.

My concern is that by introducing this intermediate type, we would be performing unneeded operations on structures that are already optimised for specific outputs.

Maybe. But that might be an unavoidable cost of a two-dimensional conversion framework that also has a sane definition of "correctness".

Should we have a single InputConverter for an Address that can read data in any format? (JSON, YAML, String, etc). Or multiple ones? One for each? The intermediary type should be the same (only the context changes).

An easier way to look at it might be to view it as a "logical" input type. This manifests in two ways: first of all, an object might have more than one valid JSON representation for various reasons (differing specs for example); second of all, a JSON representation might itself be possible with more than one different Java type (for example JSON-P versus org.json vs...). So already the problem is complex.

Logically we may know how to generically represent an Address in our "preferred" format. But to get it there could require a multitude of "horizontal" conversion steps, or otherwise a multitude of different "vertical" converters. And the same could be true on the output stage as well.

Maybe we can think about this as ConfigSource in the sense that you have multiple InputSources, each with priority and when you want to convert you either convert using the highest priority one, or you select the source you want to use?

This works with config because configuration "layering" is an understandable and useful concept. I'm not sure that layering converters is useful.

@radcortez
Copy link
Member Author

Loving this discussion, but should we try to reach a conclusion? :) (I'm pretty sure we will discuss plenty over this in the future)

We still have the open question about the intermediary type. Do you have any ideas?

@dmlloyd
Copy link

dmlloyd commented Apr 1, 2020

Loving this discussion, but should we try to reach a conclusion? :) (I'm pretty sure we will discuss plenty over this in the future)

A design discussion can come out in one of two ways: prematurely (and wrong), or correctly (however long it takes). 🙂

We still have the open question about the intermediary type. Do you have any ideas?

Well I haven't quite proposed anything yet for a reason TBH. Personally I'm not convinced that our use cases can coexist in a rational way, but am willing to continue brainstorming to see if something comes out of that.

Being "selfish" (from the perspective of config), the ideal intermediate would be like I said: numerics, string, boolean, and lists and maps. I wouldn't propose a literal object abstraction of these types, as that would amount to lots of redundant objects, but the config source would be able to get values in each of these types (putting the responsibility of conversion of just these types on the side of the config source), while the converter would accept any of these values types and handle it in the proper way (even if the proper way is to throw an exception). But there are numerous technical problems that definitely arise from this approach, so I wouldn't actually go in this direction until/unless those could be worked out. And, of course, this completely ignores any other use case, so is unlikely to be adequate in these cases.

@radcortez
Copy link
Member Author

These pretty much cover Config implicit converters.

We can assume that other complex cases will always have to be represented as String (like we have today). From the Config perspective, this is fine.

Now, is there a way that we can optimize this, when we don't control the origin? Carrying over a context, might be too heavy, plus you already have the perfect context with the structure itself. I guess that if just assume String for everything, this become way more simple.

Maybe the OutputConverter will require an option to pass in an InputConverter so they can be composed together and do everything in one go. So in a sense, you will still have unidimensional converter types, but with an option to build specialised converters that can handle these cases.

radcortez added a commit to radcortez/smallrye-converters that referenced this issue Apr 3, 2020
radcortez added a commit to radcortez/smallrye-converters that referenced this issue Apr 3, 2020
@radcortez
Copy link
Member Author

@dmlloyd I've started a draft PR here: #6

I think it might be easier to discuss looking at some code :)

@radcortez radcortez linked a pull request Jul 2, 2021 that will close this issue
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

Successfully merging a pull request may close this issue.

2 participants