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

Rename ErrorMessage to Problem, split into specific types, revise output #203

Merged
merged 29 commits into from
Dec 16, 2022

Conversation

ue71603
Copy link
Contributor

@ue71603 ue71603 commented Jun 1, 2022

  • Renamed ErrorMessage to Problem in accordance to RFC 7807, Code to Type, Text to Title and added Details and LogData
  • Added dedicated ProblemStructures for each request, enumerating the possible types, some of which were previously only defined in the documentation, some not defined at all.
  • Added the Problem element to PlaceResultStructure and TripInfoResultStructure
  • Added the Problem element to FareResultStructure so StopFareResult and StaticFareResult may also contain one and removed it from TripFareResult and MultiTripFareResult
  • Renamed some problem types (in comparison to the former documentation):
    • FARES_* to FARE_*
    • MULTIPOINTTRIP_NOTALLPOINTSCOVERED to TRIP_MULTIPOINT_NOTALLPOINTSCOVERED
    • MULTIPOINTTRIP_TOOMANYPOINTS to TRIP_MULTIPOINT_TOOMANYPOINTS

Fixes: #111

@ue71603 ue71603 added this to the v2.0 milestone Jun 1, 2022
@ue71603 ue71603 mentioned this pull request Jun 1, 2022
@sgrossberndt sgrossberndt changed the title added the error messages from the documenation Add error messages from the documentation Aug 23, 2022
@sgrossberndt sgrossberndt force-pushed the error_messeges_in_xsd branch from 575aace to 67ea1f0 Compare August 23, 2022 09:14
Copy link
Contributor

@sgrossberndt sgrossberndt left a comment

Choose a reason for hiding this comment

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

We have to discuss this change again.

  1. This list currently only consists of StopEventResponse from the documentation, which now is OJPStopEventDelivery. But apparently when I removed unused structures in 0168dcd4 it was overlooked these ErrorMessages are not part of the output at all.
    I wonder now whether this was the correct choice back then to simply remove these structures - how are error messages regarding a whole response transported now? The current state seems to only be able to transport an error message for a single StopEventResult, but not for the whole OJPStopEventDelivery. Do we have to re-add ErrorMessage at a base delivery level?

  2. Is Code in ErrorMessageStructure the correct place to document all the codes for all the responses or should they rather be documented each in the ErrorMessage where they may occur, e.g. https://github.com/VDVde/OJP/blob/changes_for_v1.1/OJP/OJP_StopEvents.xsd#L194

  3. Currently there are only the error codes of some stopEvent documented in the xsd in this commit, the documentation has a lot more, e.g. STOPEVENT_LASTSERVICEOFTHISLINE, FARES_STOPPOINTUNKNOWN, ... which are missing here.

OJP/OJP_Common.xsd Outdated Show resolved Hide resolved
@ue71603
Copy link
Contributor Author

ue71603 commented Aug 25, 2022

@sgrossberndt You have a lengthy comment here. It seems it does not relate to this change. Should we discuss it in a separate issue? Can you create it then?

We have to discuss this change again.

  1. This list currently only consists of StopEventResponse from the documentation, which now is OJPStopEventDelivery. But apparently when I removed unused structures in 0168dcd4 it was overlooked these ErrorMessages are not part of the output at all.
    I wonder now whether this was the correct choice back then to simply remove these structures - how are error messages regarding a whole response transported now? The current state seems to only be able to transport an error message for a single StopEventResult, but not for the whole OJPStopEventDelivery. Do we have to re-add ErrorMessage at a base delivery level?
  2. Is Code in ErrorMessageStructure the correct place to document all the codes for all the responses or should they rather be documented each in the ErrorMessage where they may occur, e.g. https://github.com/VDVde/OJP/blob/changes_for_v1.1/OJP/OJP_StopEvents.xsd#L194
  3. Currently there are only the error codes of some stopEvent documented in the xsd in this commit, the documentation has a lot more, e.g. STOPEVENT_LASTSERVICEOFTHISLINE, FARES_STOPPOINTUNKNOWN, ... which are missing here.

@sgrossberndt
Copy link
Contributor

No, this lengthy comment is exactly about this change. There is no use merging this pull request before we have discussed the points in this comment. In order to be able to find it for the discussion I will add it to the issue though.

@ue71603
Copy link
Contributor Author

ue71603 commented Oct 18, 2022

  • For the single result level ErrorMessage can be used.
  • We have ServiceDelivery/ErrorCondition for general high level errors:
    image
  • We also have OJPExchancePointDelivery/ErrorCondition:
    image
    This is still very abstract.

One idea would be for Delivery level errors would be to add a ServiceDeliveryErrorGroup. It that for simplicities sake, I would like to put multiple ErrorMessageStructures. However, this is not possible, because DeliveryStatusGroup belongs to Siri

I will try to put it into the ServiceResponseContextGroup
image

Then I would like to formalize ErrorMessageStructure (but not having a different one per request/response:

  • Code as Enum
  • Text for the description
  • LogDatan as xs:string

The code is then expanded from

  • OJP specification
  • SIRI
  • TRIAS

@ue71603
Copy link
Contributor Author

ue71603 commented Oct 18, 2022

  • SIRI is already included in the ErrorCondition, but was not fully helpful
  • Started copying codes from the specification. @trurlurl could you continue? The eumeration is ErrorCodeEnumeration in OJP_Commons.xsd and in the specification you search for "possible errors"
  • @sgrossberndt It seems to me that TRIAS uses the same codes in the specification document as OJP. But I only have the 1.2 file. Is the 1.4 out already?

@sgrossberndt sgrossberndt force-pushed the error_messeges_in_xsd branch from 755bb0f to c732bf9 Compare October 19, 2022 07:07
@sgrossberndt sgrossberndt force-pushed the error_messeges_in_xsd branch from c291fcf to b541ecb Compare October 19, 2022 07:14
@sgrossberndt
Copy link
Contributor

You can find TRIAS releases at https://github.com/VDVde/TRIAS/releases, most recent is v1.3 but v1.4 will be released in the near future

@herlitze
Copy link
Contributor

herlitze commented Nov 9, 2022

I am also torn. I see the proposed solution with a structure but subcategories in the enum name as a good compromise

skinkie
skinkie previously approved these changes Nov 24, 2022
Copy link
Contributor

@skinkie skinkie left a comment

Choose a reason for hiding this comment

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

I still wonder why we change ErrorMessage to "Problem" maybe this choice of words can be explained.

@ue71603
Copy link
Contributor Author

ue71603 commented Nov 24, 2022

Part of the commit message: https://datatracker.ietf.org/doc/html/rfc7807 and also. Errorstructures by Service (and perhaps a general one) and have a complete list as annex to the standard rendered by the documentation engine.

@ue71603
Copy link
Contributor Author

ue71603 commented Dec 6, 2022

@sgrossberndt updated: Is this ok for you now?

sgrossberndt and others added 3 commits December 7, 2022 10:13
# Conflicts:
#	docs/generated/OJP.html
* `OJP_Common.xsd` now only contains the `ProblemDetailGroup` shared by all `ProblemStructure`s
* Renamed `ProblemStructure` to `OJPGenericProblemStructure` and moved it to `OJP_RequestSupport.xsd` as it is only used there
* Moved the `Problem` element to `FareResultStructure` so `StopFareResult` and `StaticFareResult` may also contain a problem and removed it from `TripFareResult` and `MultiTripFareResult`
* Moved the problem definitions to the bottom of each file and have them in the same order everywhere
* Renamed some problem types:
  * `OTHER` to `OJPGENERIC_OTHER`
  * `FARES_*` to `FARE_*`
  * `MULTIPOINTTRIP_NOTALLPOINTSCOVERED` to `TRIP_MULTIPOINT_NOTALLPOINTSCOVERED`
  * `MULTIPOINTTRIP_TOOMANYPOINTS` to `TRIP_MULTIPOINT_TOOMANYPOINTS`
* Added the missing `Problem` element for the `TripInfoProblemStructure`
* fixed some typos
@sgrossberndt sgrossberndt changed the title Add error messages from the documentation Rename ErrorMessage to Problem, split into specific types, revise output Dec 7, 2022
sgrossberndt
sgrossberndt previously approved these changes Dec 7, 2022
Copy link
Contributor

@sgrossberndt sgrossberndt left a comment

Choose a reason for hiding this comment

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

Proposal for commit message when squashing:

  • Renamed ErrorMessage to Problem in accordance to RFC 7807, Code to Type, Text to Title and added Details and LogData
  • Added dedicated ProblemStructures for each request, enumerating the possible types, some of which were previously only defined in the documentation, some not defined at all.
  • Added the Problem element to PlaceResultStructure and TripInfoResultStructure
  • Added the Problem element to FareResultStructure so StopFareResult and StaticFareResult may also contain one and removed it from TripFareResult and MultiTripFareResult
  • Renamed some problem types (in comparison to the former documentation):
    • FARES_* to FARE_*
    • MULTIPOINTTRIP_NOTALLPOINTSCOVERED to TRIP_MULTIPOINT_NOTALLPOINTSCOVERED
    • MULTIPOINTTRIP_TOOMANYPOINTS to TRIP_MULTIPOINT_TOOMANYPOINTS

herlitze
herlitze previously approved these changes Dec 12, 2022
OJP/OJP_Places.xsd Outdated Show resolved Hide resolved
@sgrossberndt sgrossberndt dismissed stale reviews from herlitze and themself via 8ac1783 December 15, 2022 14:42
@sgrossberndt sgrossberndt merged commit de4cf82 into changes_for_v1.1 Dec 16, 2022
@sgrossberndt sgrossberndt deleted the error_messeges_in_xsd branch December 16, 2022 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants