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

[WIP] ua: export NodeID fields to make it easier marshallable #250

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

birdayz
Copy link

@birdayz birdayz commented Jul 24, 2019

I'm currently working on a OPCUA PubSub (Part 14) implementation. For one step, where the message consumer has to store MetaData persistently, so we can reference it later when a DataFrame arrives, i need to Marshal the MetaData. I chose to do it with JSON. The default JSON Marshaler can't store the NodeID, as its fields are unexported. By exporting them, it's possible to fully marshal the metadata. (Which contains stuff like StuctureDescription).

I'm open for other solutions; one possibility would be implementing MarshalJSON() , UnmarshalJSON() with a custom unexported struct with these exported fields.

Another option is that i implement my own custom data structure; but with so many nested fields it's messy and i will pretty much create a clone of gopcua's structs with a minor difference. Not very good, i would prefer to have it upstream.

for opcua pubsub (Part 14), a consumer needs to store metadata frames.
marshalling them as a JSON is a possible solution; which is not possible without
custom types if NodeID fields are unexported. With the fields being exported,
it's possible to marshal it with default marshaling behavior.
@kung-foo
Copy link
Member

related: #243

@kung-foo
Copy link
Member

As always with these kinds of network/byte structs, there are other relationships and meanings baked into the raw values. For example, should the JSON contain the NodeIDType as a distinct value (it is part of mask)? And from the API side, methods like SetIntID provide logic and checks for valid values. If we make the field public, then I would expect that we would remove the related setters/getters, otherwise it is not clear to the user which approach they should use.

@magiconair
Copy link
Member

I would prefer not to export the fields since they are an implementation detail of how to represent a NodeID. They are not the node id itself. I think using MarshalJSON is better and why don't we just use the string representation, e.g. {"node":"ns=4;i=2344"} ?

@magiconair
Copy link
Member

@birdayz unrelated question: are you working on adding PubSub support to gopcua? That would be cool.

@birdayz
Copy link
Author

birdayz commented Jul 24, 2019

Yes. It's currently internal code but i will will upstream it in the next weeks.
Works already, currently doing the last polishing..

@kung-foo
Copy link
Member

Regarding the string representation, that is how I did it for our Sub code: https://gist.github.com/kung-foo/b184a432de2e3063f891f96d42b25795#file-monitor-go-L98

@magiconair
Copy link
Member

@birdayz After thinking about this for a bit why doesn't the NodeID.String() method work for what you're trying to do?

@birdayz
Copy link
Author

birdayz commented Jul 26, 2019

yeah i guess it will work. Anyhow, i would need to add the MarshalJSON / UnmarshalJSON functions to the NodeID type. which is OK.

i just didn't see a function to construct a NodeID type from the string when unmarshaling from JSON(so the opposite direction of what String() is doing). Or did i just miss that?

@magiconair
Copy link
Member

There should be ParseNodeID

@magiconair
Copy link
Member

@birdayz
Copy link
Author

birdayz commented Jul 27, 2019

Looks good. Will update the PR next week :)

@kung-foo
Copy link
Member

regarding the JSON structure, part 6 of the spec (5.4.2.10) says that the format should be:

type JNodeID struct {
	IDType    int         `json:"IdType,omitempty"`
	ID        interface{} `json:"Id"`
	Namespace int         `json:",omitempty"`
}

see https://play.golang.org/p/wbTF-xAYaR8

@magiconair
Copy link
Member

If we follow down the path of implementing the official JSON mapping via MarshalJSON on the type then we probably need to add alias types for all standard scalar types since float32, float64, time.Time require custom marshaling.

I'm not sure I'd like that since this might a lot of unnecessary casting between Float and float32, DateTime and time.Time and so forth. The reason I wrote the reflection codec was to be able to use the native Go types.

A standalone JSON encoder might be a better approach since it provides more flexibility on the format and the Go types we're mapping to. It would hide the complexity of the protocol better.

@kung-foo
Copy link
Member

I agree that all the casting would be a pain, and probably brittle. On the other hand, the JSON should either match the spec exactly, or this project should explicitly state that the JSON is not compatible. It's a mess either way.

@magiconair
Copy link
Member

magiconair commented Jul 30, 2019 via email

@kung-foo
Copy link
Member

Do you mean an encoder/decoder that doesn't live on the NodeID struct?

@magiconair
Copy link
Member

magiconair commented Jul 31, 2019

Most of the OPC/UA types have a JSON representation that is compatible with what the Go JSON encoder would generate from the current structs. However, there are some exceptions.

For example, ByteString is a []byte but should be encoded as a base64 encoded string. With a ByteString type this would be easy but that would mean casting all []byte to ByteString which might be tolerable.

DateTime is encoded to an RFC3339 format which maps to time.Time so that should be OK.

int64 and uint64 should be encoded as strings and there is support in the std JSON encoder for this via the json:",string" tag.

All other numbers should be encoded as JSON numbers. However, special values like +Inf, -Inf, and NaN should be encoded as strings with the respective value. The std JSON encoder errors on +Inf (https://play.golang.org/p/vm3nJ4Dp3we)

5.4.2.3 Integer
Integer values other than Int64 and UInt64 shall be encoded as a JSON number.
Int64 and UInt64 values shall be formatted as a decimal number encoded as a JSON string (See the XML encoding of 64-bit values described in 5.3.1.3).

5.4.2.4 Floating point
Normal Float and Double values shall be encoded as a JSON number.
Special floating-point numbers such as positive infinity (INF), negative infinity (-INF) and not-a- number (NaN) shall be represented by the values “Infinity”, “-Infinity” and “NaN” encoded as a JSON string. See 5.2.2.3 for more information on the different types of special floating-point numbers.

Structs with an EncodingMask like NodeID need to be special-cased anyway.

So how would we encode/decode a struct that has a float64?

  1. We change the type in the struct to a type alias ua.Double with custom marshaling functions. Then all Go code has to cast float64 to ua.Double back and forth.
  2. We fork the json encoder to add some special cases. However, even though the encoder is stable in terms of features there is still active development in terms of performance and bugs. We could automate this probably but still.
  3. We create a JSON version of the struct which uses the custom types so that we can use the std JSON encoder. This means defining twice as many types (could be private and they are generated), copying values back and forth. This might cause more pressure on the GC unless the objects are all on the stack. We could write a recursive function using reflection which copies between the types since the structure and the field names are the same. (https://play.golang.org/p/7ENGZP7zxkF)
  4. We try to use an alternative JSON encoder like https://github.com/mailru/easyjson, https://github.com/francoispqt/gojay, https://github.com/json-iterator/go and see if they give us more control over the edge cases.

It would be nice if the std JSON encoder had hooks for the standard types which we could replace but it doesn't.

I think option 3 could work since it uses the std json encoder and the custom types but hides all that from the user.

@magiconair
Copy link
Member

https://play.golang.org/p/7ENGZP7zxkF for option 3 also gives you an idea about the amount of casting and why I'd like to avoid that.

Having said that, I think custom (Un)MarshalJSON functions for NodeID which format according to the OPC/UA JSON representation is probably the right approach. (@birdayz)

@birdayz
Copy link
Author

birdayz commented Jul 31, 2019

Thanks for having the discussion. this was also my preferred alternative, I'm happy to work on that.

@kung-foo
Copy link
Member

@magiconair Would you expose (public) the JNodeID (or equivalent)? I understand your big concern is the low-level type conversion. Mine is the schema conversion which must be maintained.

func (n *NodeID) AsJNodeID() *JNodeID {
	return &JNodeID{
		Namespace: n.nid, // easy
		ID: ... // not easy
		...
	}
}

func (n *NodeID) MarshalJSON() ([]byte, error) {
	return json.Marshal(n.AsJNodeID())
}

@magiconair
Copy link
Member

I would hide it for now. If we change our minds we can. Otherwise, the horse is out of the barn and users may get confused which one to use.

@birdayz
Copy link
Author

birdayz commented Aug 20, 2019

FYI i currently don't have time to pursue this due to time pressure at work for other stuff :] i didn't forget about and am still planning to open source the pubsub impl. and work on this. i hope to pick it up in autumn again. sorry!

@birdayz birdayz changed the title ua: export NodeID fields to make it easier marshallable [WIP] ua: export NodeID fields to make it easier marshallable Aug 20, 2019
Base automatically changed from master to main March 26, 2021 00:58
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 this pull request may close these issues.

3 participants