Conversation
| /// Type for EPP XML `<fee:delete>` element | ||
| /// | ||
| /// Used in <delete> commands. | ||
| #[derive(Debug)] | ||
| pub struct Delete; | ||
|
|
||
| // TODO: Find solution to avoid these. | ||
| // This is needed to avoid writing an emoty <delete> tag into <extension> | ||
| // instant-epp currently requires ToXml impls for all command types | ||
| // and we need a way to make infer the response type added by the extension | ||
| // for this command. | ||
| // This will still add the <extension> tag currently, but it will be empty. | ||
| // Some EPP servers are not happy about en empty <extension> tag though. | ||
| impl ToXml for Delete { | ||
| fn serialize<W: std::fmt::Write + ?Sized>( | ||
| &self, | ||
| _field: Option<instant_xml::Id<'_>>, | ||
| _serializer: &mut instant_xml::Serializer<W>, | ||
| ) -> Result<(), instant_xml::Error> { | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
The current request response mapping falls short on some extensions. This is one of them.
I think we talked about this in the past.
For and The fee extension only adds elements to the response.
This means you get elements in <extension> if you enabled an extension during login.
With instant-epp you currently need to have a type implementing Extensions and Transaction to correctly map the response to a request.
Using EppClient::transact the input is used to construct RequestData<'c, 'e, Cmd, Ext>.
Are we able to use expand the Extension trait to have RequestData<'c, 'e, Cmd, Ext> construct extension: None internally if we are not adding any elements to the request?
This would help when there is a direct mapping from request to response for an extension.
This still leaves a lot of responsibility to the user of instant-epp. E.g. for each call to transact, you need to specify all possible extension data types enabled during login.
Having a registry of enabled extensions on a client seems like the better idea.
Throwing some ideas into the room (haven't played around with this idea yet):
EPP is based on the core commands; I haven't encountered an extension that adds commands.
Thus, extensions could provide all request and response data types via similar enums (let's call them FeeCommand and FeeResponse).
We then provide one enum of all supported in-tree extensions (let's call this SupportedExtensions for now).
Response will change to Response<E = SupportedExtensions> {... extension: Vec} You are then able to extract your desired extension data from a response if it was part of the response.SupportedExtensions` is used as a default for E to be able to support custom enums for out of tree extensions.
Obv. this is lacking some ergonomic helpers. E.g. We could expect FeeResponse to implement ExtensionResponse::as_info(&self) -> Option<&InfoData>
There was a problem hiding this comment.
A short term solution is something like this: valkum@2384ad6
I guess it makes sense to move this to an issue to discuss further.
| @@ -0,0 +1,321 @@ | |||
| //! XSD Duration format | |||
There was a problem hiding this comment.
The spec uses some XSD "std" parts. Should we add this to instant-xml or an instant-xml-std?
There was a problem hiding this comment.
During my testing I used std::time::Duration and tried to use this as a remote type. See djc/instant-xml#110, but the two duration formats are not really compatible. In my eyes, this ISO based format is highly flawed. XSD says each month is 30 days and each year has 12 months. Thus, I used this directly here. It is up the user to interpret this according to their business logic. We could also fall back to just store String if we don't care at all.
| // We need to implement FromXml manually here because FromXml currently contains bugs. | ||
| // See https://github.com/djc/instant-xml/issues/113 | ||
| // Even when used with named fields this the derive macro results in | ||
| // `Xml(DuplicateValue("TransformResultType::currency"))`` |
There was a problem hiding this comment.
This might be worth fixing in instant-xml first. This still fails for the case where TransformResultType is marked as transparent and used in a named field.
| // There seems to be an incopatiblity with `serialize_with` and `direct` | ||
| // We need direct for the FromXml derive to work correctly, but you cannot | ||
| // combine this with `serialize_with`. | ||
| impl ToXml for FeeType { |
There was a problem hiding this comment.
I haven't looked into this in detail and if we can allow this in the derive macro.
valkum
force-pushed
the
fee_01
branch
2 times, most recently
from
c40479f to
4284b7f
Compare
|
Sorry, I've been very busy with paid work and that will likely remain true for the next few weeks. Keeping this on my list for when I have bit more availability. |
| /// Scalar enum for fee:currency | ||
| #[derive(Debug, Clone, Copy, PartialEq, FromXml, ToXml)] | ||
| #[xml(scalar, rename = "currency", rename_all = "UPPERCASE", ns(XMLNS))] | ||
| pub enum Currency { |
There was a problem hiding this comment.
We might want to use celes here instead.
2 participants
Hey,
here is some work I recently did in-tree.
This adds RFC 8748
urn:ietf:params:xml:ns:epp:fee-1.0support.There are some outstanding issues we might want to solve here for general use.
I will add my remarks to the respective parts via GitHub review comments.