Skip to content

OJP 2.0 #3

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

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Conversation

leonardehrenfried
Copy link

@leonardehrenfried leonardehrenfried commented Feb 17, 2025

This adds the upgrade to OJP 2.

It represents a work in progress but I have tested it for a little while and it does what I need it to do.

Are you interested in pulling these changes into this repo?

If not I'm happy to host it elsewhere.

One perhaps interesting addition is XmlDateTime which is a wrapper for the fact that xml:dateTime can bei either zoned or local. This forces you to deal with that fact.

I'm aware that the pretty namespace abbreviation are not necessary but they help me keep track of which namespace abbreviation is which.


<jxb:bindings schemaLocation="./src/main/resources/xsd/2.0/OJP.xsd">
<jxb:schemaBindings>
<jxb:package name="de.vdv.ojp20" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not keep it de.vdv.ojp and have the package version management do the heavy lifting?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you want to use both? In my case it's for OTP which often has very long backwards-compatibility guarantees.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your case it does not matter, since OTP never supported OJP. Now implementing 1 sounds not the right path forward.

To answer your question:
https://boyl.es/post/two-versions-same-library/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about OJP 3?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing. New library version of added, if required an other alias is used. Reality check: given the history of OTP 1.x it is more likely that again the baby is trown away with the bathwater than the support of multiple versions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SKI+ OJP active runs with OJP 1 only, OJP passive can deal with OJP 1 and OJP 2. It would be easiear to generate a versioned namespace (or any other concept in the library), its just target code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phirzel I highly doubt that you would find a versioned namespace 'easier', because that would mean that all the Java code for processing must be duplicated.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well in my not released clone https://github.com/openTdataCH/ojp-adapter/tree/main/ojp-java-model I still use java-model 1.0.3.1 for Rutebanken libs, but generate/clone my own OJP 2.0 target java-model additionally.
I am thinking of not using the ojp-java-model any longer, it's a mess with mentioned siri code and Jaxb context for OJP 1. In the end I use a Journey-Service which interacts both with OJP active and passive, therefore I would need both java-models resp. OJP-Adapter versions v0.9 (OJP 1 based) and the next release which would support OJP 2 parallel. Do I really need to deal with siri and rutebanken and whatever else keeps as is in OJP 1 and 2 xsd/java? I would prefer the lib provider to solve such time consuming things for all users once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For every class you touch in your code that happens in version 1, you would write new code that imports class version 2. Are you in agreement with that?

Copy link

@phirzel phirzel Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my case a request to v1 or v2 results into an appropriate v1 or v2 response. My "OJP-Adapter" abstracts the mapping to ojp-java-model. The caller of my OJP-Adapter dictates whether OJP active and/or passive is the target system, both Adapter (or its method versions) reply the same Transmodel implementation model in my case, so the semantic layer abstracts OJP versions. (Keeping the very same namespace leads to maven and java dependency mess, since duplicated code is most probably involved in ojp-java-model v1.0.3.1 and v2)

</jxb:schemaBindings>
</jxb:bindings>
<!-- the netex-java-model already defines the siri types in uk.org.siri which leads to clashes.
therefore we put the generate siri types into a sub-package of de.vdv.ojp -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also do not like this argument. If this is a problem, it should be fixed by a separate SIRI package.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is not so much the actual types, which are very similar (identical?), but their annotation to which schema and therefore JAXB context they belong. I see it more of a problem with JAXB rather than the generated code, unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I have had in practice was that if you for example when a product is developed that integrates NeTEx and a SIRI stream, and then produces OJP output, that would require to manually assign all properties of object A to "incompatible same object" B. What would be your solution for you to copy over the values?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the OTP world this would go through the internal model, so there are a few mapping layers in between anyway. We wouldn't even notice the madness. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I know 'many vendors' go through internal models and are not able to produce an audit trail of the data flow. But that is for a conversion service a bad practise. Since we currently have tagged an upstream SIRI in the OJP repository, I think walking the split package path might be a better idea, that having said, given that it is a tagged upstream (unlike with NeTEx) it should not collide in the first place since it is equal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leonardehrenfried I think i want to consult @sgrossberndt on this.

@leonardehrenfried
Copy link
Author

leonardehrenfried commented Mar 12, 2025

For the time being, I have published this to Maven Central under the org.opentripplanner group id: https://mvnrepository.com/artifact/org.opentripplanner/ojp-java-model/2.0.0

Let me know if you'd like me to continue trying to get this merged with your repo or if you think our requirements are just too different to be reconciled.

@skinkie
Copy link
Contributor

skinkie commented Mar 12, 2025

I have sent a mail to the other Stephan.

@@ -51,21 +51,21 @@

<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<jdk.version>1.8</jdk.version>
<jdk.version>17</jdk.version>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I would prefer 21)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the benefit for increasing it?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Lifecycle matter, 17 seems old already)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also like to encourage people to update their Java versions regularly so I would be ok with 21.

Copy link
Contributor

@joelhaasnoot joelhaasnoot Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be devil's advocate - why? This is a library, what if a consumer is still on 17 for a large enterprise project for example? In the Scala ecosystem 17 is still widely used for example

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then they cannot use this library and have to upgrade to 21, which would be a good idea anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there is no technical reason then

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but we don't owe anyone any specific compatibility. Do you have a use case for Java 17?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We strive for adoption.

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.

5 participants