Skip to content

Conversation

@mwwoda
Copy link
Contributor

@mwwoda mwwoda commented Jul 30, 2021

From owners of FileRequest API
This field is intended to not have host name. As there are multiple scenarios for customers (app vs. ent and subdomains), these can change at runtime. We decided to not store or return hostname prior to release.

@mwwoda mwwoda requested review from PJSimon and mhagmajer July 30, 2021 09:04
@mwwoda mwwoda linked an issue Jul 30, 2021 that may be closed by this pull request
@coveralls
Copy link

coveralls commented Jul 30, 2021

Pull Request Test Coverage Report for Build 2550

  • 10 of 19 (52.63%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 67.518%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/main/java/com/box/sdk/BoxAPIConnection.java 2 4 50.0%
src/main/java/com/box/sdk/BoxFileRequest.java 8 15 53.33%
Totals Coverage Status
Change from base Build 2546: -0.02%
Covered Lines: 6660
Relevant Lines: 9864

💛 - Coveralls

* @return the date at which this task is due.
*/
public URL getUrl() {
public String getUrl() {
Copy link
Contributor

@PJSimon PJSimon Jul 30, 2021

Choose a reason for hiding this comment

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

This is a breaking change, so I'm wondering if there's another approach we could take, for example generating the full URL from the partial path that comes back from the service.

  • Is there anyway to the get the first part of the URL by calling the Box API? Sorry, don't know that off the top of my head.
  • Maybe it could be inferred from a property of the BoxFolder?
  • Or could a config parameter in the SDK be used, similar to:
    private static final String DEFAULT_BASE_URL = "https://api.box.com/2.0/";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is breaking change from the SDK perspective, but according to service owners this change was done prior to the release. So technically, we could probably slide it off as a bugfix (incorrect model mapping), but if we don't want to break existing customers we could try to take the approach you proposed and build a full URL there.

  • I don't think we have a possibility to get the first part of the URL from the Box API.
  • I don't see any property that could be inferred from BoxFolder.
  • We could use a config parameter, but we should add a new one as we don't have one for https://app.box.com used here. We should also make it configurable. According to the workflow team - the base URL can differ (different subdomains for enterprises for different customers). Maybe we can expose two properties here? Existing one with full URL and new one, only with path in case client needs to build their url differently at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to use the third approach with config parameter while still giving ability to change the base url manually.

Copy link
Contributor

@mhagmajer mhagmajer left a comment

Choose a reason for hiding this comment

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

lgtm

Will leave for @PJSimon to sign off on this

@mwwoda mwwoda merged commit 8094c85 into main Aug 6, 2021
@mwwoda mwwoda deleted the fix-file-requests branch August 25, 2021 14:14
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.

BoxFileRequest is throwing com.box.sdk.BoxDeserializationException

5 participants