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

Improve JSON services documentation and other enhancements #6777

Merged
merged 50 commits into from Apr 18, 2019

Conversation

Projects
None yet
4 participants
@rhwood
Copy link
Contributor

commented Apr 8, 2019

This was originally intended to just improve the JSON services documentation and fix the JSON jQuery Javascript library, but has expanded a little beyond that:

  • Consolidate JSON protocol documentation in one place to remove contradictory documentation
  • Make JSON jQuery library follow JSON protocol documentation (fixes #6723, fixes #6765)
  • Ensure JSON services do not listen to unrequested objects (fixes #6735)
  • Allow property change listeners for NamedBeans to listen to only specific properties
  • Address some warnings from compilers and static analysis
  • Uncomment commented out test code in AbstractTimeServerTestBase and fix revealed bad code in SRCP server

rhwood added some commits Mar 27, 2019

fix: Use temporary folder for profiles
If this does not fix #5642, the correct fix would be to set the settings path to the temporary folder in `setUp()` and call `JUnitUtil.resetFileUtilSupport()` in `tearDown()`

Fix #5642
feat: Allow get parameters to be used
Allow all JSON services to use parameters (HTTP URL parameters in the RESTful server or as contents of a data body in sockets) to influence the data that is returned.

Use the `server=true|false` parameter in the schema server to specify if both server and client schemas are returned (no parameter) or only the server schema is returned (`true`) or only client schema is returned (`false`).
fix: use JSON.UNKNOWN for Block.UNKNOWN in JSON message
Since JSON.UNKNOWN is 0, JSON clients can use this as a truthy value
chore: save work in progress
This commit will break HTML and Javadoc tests, but I need to save work in progress.

@rhwood rhwood changed the title Improve JSON services documentation and other enhancements 🚧 Improve JSON services documentation and other enhancements Apr 8, 2019

@rhwood

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Javadocs and Help content expected to cause CI errors until complete, but this PR is open for comment while working on that.

@rhwood rhwood changed the title 🚧 Improve JSON services documentation and other enhancements Improve JSON services documentation and other enhancements Apr 11, 2019

rhwood added some commits Apr 11, 2019

fix: Relax requirement that JSON objects use system names
It remains recommended that system names be used for all access, but classes descending from JsonNamedBeanHttpService only require system names to create new objects, not to retrieve existing objects; it remains possible that some clients may mangle a user name such that JMRI is incapble of matching it, hence the recommendation.
@rhwood

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Travis CI GUI tests for 9707179 failed on:

[INFO] Running jmri.jmrit.display.layoutEditor.LayoutEditorLoadAndStoreTest
ERROR - match failed in LoadAndStoreTest: [main] jmri.configurexml.LoadAndStoreTest.checkFile()
ERROR -     file1:line 10: "  <turnouts class="jmri.jmrix.internal.configurexml.InternalTurnoutManagerXml">" [main] jmri.configurexml.LoadAndStoreTest.checkFile()
ERROR -     file2:line 10: "  <sensors class="jmri.jmrix.internal.configurexml.InternalSensorManagerXml">" [main] jmri.configurexml.LoadAndStoreTest.checkFile()
ERROR -   comparing file1:"/home/travis/build/JMRI/JMRI/java/test/jmri/jmrit/display/layoutEditor/loadref/LayoutEditorTest.xml" [main] jmri.configurexml.LoadAndStoreTest.checkFile()
ERROR -          to file2:"/home/travis/build/JMRI/JMRI/temp/temp/LayoutEditorTest.xml" [main] jmri.configurexml.LoadAndStoreTest.checkFile()
[ERROR] Tests run: 2, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.903 s <<< FAILURE! - in jmri.jmrit.display.layoutEditor.LayoutEditorLoadAndStoreTest
[ERROR] loadLoadStoreFileCheck[java/test/jmri/jmrit/display/layoutEditor/load/LayoutEditorTest.xml (pass=true)](jmri.jmrit.display.layoutEditor.LayoutEditorLoadAndStoreTest)  Time elapsed: 0.892 s  <<< FAILURE!
org.junit.ComparisonFailure: expected:<  <[turnouts class="jmri.jmrix.internal.configurexml.InternalTurnout]ManagerXml">> but was:<  <[sensors class="jmri.jmrix.internal.configurexml.InternalSensor]ManagerXml">>
	at org.junit.Assert.assertEquals(Assert.java:115)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at jmri.configurexml.LoadAndStoreTestBase.checkFile(LoadAndStoreTestBase.java:200)
	at jmri.configurexml.LoadAndStoreTestBase.loadLoadStoreFileCheck(LoadAndStoreTestBase.java:281)
[...]
[ERROR] loadLoadStoreFileCheck[java/test/jmri/jmrit/display/layoutEditor/load/LayoutEditorTest.xml (pass=true)](jmri.jmrit.display.layoutEditor.LayoutEditorLoadAndStoreTest)  Time elapsed: 0.898 s  <<< FAILURE!
java.lang.AssertionError: Unexpected ERROR or higher messages emitted: null

which I don't think is related to this PR.

If that test can be run again without failure, I think this is ready to review and merge.

@pabender

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Restarted Travis headed ( see note from @rhwood above )

@rhwood

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@pabender Thank you!

@bobjacobsen

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@rhwood Two sets of issues have arisen:

  • @dsand47 The new code added to master in #6752 has in compile-time errors which I'll append below. This is because this PR adds some must-implement methods that #6752 was written before.

  • @danielb987 removed the jmri.managers.GeneralManager class along with jmri.managers.GeneralManagerTest, but jmri.managers.GeneralManagerTest had been touched, hence a conflict. java/test/jmri/managers/GeneralManagerTest.java just needs to be removed.

Unfortunately, the 2nd item is keeping CI from rerunning unmerged, and I don't know enough to fix the first item and merge without CI.

@bobjacobsen

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

compile:
    [javac] Compiling 4357 source files to /Users/jake/Documents/Trains/JMRI/projects/JMRI/target/classes
    [javac] warning: Supported source version 'RELEASE_7' from annotation processor 'org.netbeans.modules.openide.util.ServiceProviderProcessor' less than -source '1.8'
    [javac] warning: Supported source version 'RELEASE_7' from annotation processor 'org.netbeans.modules.openide.util.NamedServiceProcessor' less than -source '1.8'
    [javac] /Users/jake/Documents/Trains/JMRI/projects/JMRI/java/src/jmri/jmrit/ctc/NBHSensor.java:69: error: NBHSensor is not abstract and does not override abstract method addPropertyChangeListener(String,PropertyChangeListener,String,String) in NamedBean
    [javac] public class NBHSensor implements Sensor {
    [javac]        ^
    [javac] /Users/jake/Documents/Trains/JMRI/projects/JMRI/java/src/jmri/jmrit/ctc/NBHTurnout.java:32: error: NBHTurnout is not abstract and does not override abstract method addPropertyChangeListener(String,PropertyChangeListener,String,String) in NamedBean
    [javac] public class NBHTurnout implements Turnout {
    [javac]        ^
    [javac] /Users/jake/Documents/Trains/JMRI/projects/JMRI/java/src/jmri/jmrit/ctc/NBHSignalHead.java:23: error: NBHSignalHead is not abstract and does not override abstract method addPropertyChangeListener(String,PropertyChangeListener,String,String) in NamedBean
    [javac] public class NBHSignalHead extends NBHAbstractSignalCommon implements SignalHead {
    [javac]        ^
    [javac] /Users/jake/Documents/Trains/JMRI/projects/JMRI/java/src/jmri/jmrit/ctc/NBHSignalMast.java:26: error: NBHSignalMast is not abstract and does not override abstract method addPropertyChangeListener(String,PropertyChangeListener,String,String) in NamedBean
    [javac] public class NBHSignalMast extends NBHAbstractSignalCommon implements SignalMast {
    [javac]        ^
    [javac] 4 errors
    [javac] 2 warnings
@dsand47

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

addPropertyChangeListener

Commit 21ee817 in branch das_test_ctc_json in my repository should fix the NBH method compile errors.

I don't know if this is the best fix but it compiles, works with a minimal check, the CTC package test works and spot bugs was happy.

I don't know if I should make this a PR or if someone wants to copy the commit.

@rhwood

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

@dsand47 I added complete methods in 8be77eb, not just enough to compile. We should consider making an abstract parent class to all your NBH* classes that uses generics to reduce the amount of code duplication.

@rhwood

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

@bobjacobsen I think this is mergeable now.

@bobjacobsen bobjacobsen merged commit 47732f2 into JMRI:master Apr 18, 2019

4 checks passed

WIP Ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 50.449%
Details
@dsand47

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

@dsand47 I added complete methods in 8be77eb, not just enough to compile. We should consider making an abstract parent class to all your NBH* classes that uses generics to reduce the amount of code duplication.

@rhwood I am going to work with the creator of the NBH classes to refactor them. They provide access to the beans using named bean handles. There was no reason to specify implements in the class definition. This will eliminate 90% of the code in the NBH classes since only a few of the methods are actually used. And their only purpose is to pass the request to the bean referenced by the named bean handle.

@bobjacobsen

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@dsand47 Is there any chance the usual NamedBean interfaces (i.e. Sensor) can have methods added to them so that the NBH classes are not necessary? Shadow classes like this are often a long-term integration problem, as more and more places in the code has to be modified to deal with them.

@dsand47

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

@dsand47 Is there any chance the usual NamedBean interfaces (i.e. Sensor) can have methods added to them so that the NBH classes are not necessary? Shadow classes like this are often a long-term integration problem, as more and more places in the code has to be modified to deal with them.

@bobjacobsen While these were implemented as shadow classes, I don't think they actually need to be shadow classes.

The primary purpose of the NBH classes is to move error handling and try/catch processing out of the application code. The method call is passed to the real object using NamedBeanHandle.getBean(). If an error occurs for a request, a safe value is returned, such as INACTIVE for sensor.getKnownState().

I have created a branch, das_change_nbh, that removes the implements clause from the class definition and removes all of the methods except for the ones the CTC application actually uses.

@bobjacobsen bobjacobsen added this to the 4.15.6 milestone May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.