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

fixing .geojson file not supported with mac os #10082

Conversation

congchen1101
Copy link
Contributor

FixBug .geojson file not supported in macos.

  • In macOS .geojson file will be identified as type of "application/geo+json". So in checkFileType and readFile functions, they will not go into MIME_LOOKUPS list to find the corresponding type.
  • Check first in MIME_LOOKUPS list and then the file.type will solve this problem. On behalf of DB Systel GmbH

Description

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Issue

What is the current behavior?

#10081

What is the new behavior?
now import geojson is also supported with macOS

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

Other useful information

“On behalf of DB Systel GmbH”

FixBug .geojson file not supported in macos.
- In macOS .geojson file will be identified as type of "application/geo+json". So in checkFileType and readFile functions, they will not go into MIME_LOOKUPS list to find the corresponding type.
- Check first in MIME_LOOKUPS list and then the file.type will solve this problem.
On behalf of DB Systel GmbH
@tdipisa tdipisa requested review from MV88 and offtherailz and removed request for MV88 March 19, 2024 12:06
@tdipisa tdipisa added this to the 2024.01.01 milestone Mar 19, 2024
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.
This quick fix could be ok, but the most correct solution could also be to support the mime-type in the code.
I'd also add a unit test to preserve this functionality across maintainance.

@tdipisa tdipisa changed the title #10081 fixing .geojson file not supported with mac os Mar 26, 2024
@congchen1101
Copy link
Contributor Author

congchen1101 commented Apr 8, 2024

Thank you for your suggestions. While attempting to add unit tests, I discovered that unit tests for reading geojson files already existed. This led me to wonder why those tests were passing despite the issues at hand. Upon further investigation into the existing tests in processFiles-test.jsx, I noticed that most of them utilize the getFile method from testData.js. This method employs an axios.get call to read files from local test resources, attempting to extract the response-type value from the headers as the type to create a new Blob object, which is then used to create a File object.

# code before fix
export const getFile = (url, fileName = "file") =>
    Rx.Observable.defer( () => axios.get(url, {
        responseType: 'arraybuffer'
    }))
        .map( res => {
            return new File([new Blob([res.data], {type: res.headers['response-type']})], fileName);
        });

However, I encountered two main issues.

  1. The res.headers does not contain a response-type, meaning that all newly created Blob objects lack a type attribute. This flaw explains why all current tests could pass: the absence of a type attribute in the checks leads the code to searching file extend in MIME_LOOKUPS table for the corresponding type.
  2. Even after adjusting response-type to content-type, which allowed the new Blob object to successfully have a type property, the new File object still failed to inherit the Blob's type attribute.

Given these observations, I opted to directly assign the content-type as the type value when creating the new File object, rather than doing so during the Blob object's creation. This approach ensures that the intended file type is recognized correctly in tests.

# code after fix
export const getFile = (url, fileName = "file") =>
    Rx.Observable.defer( () => axios.get(url, {
        responseType: 'arraybuffer'
    }))
        .map( res => {
            const contentType = res.headers['content-type'] === 'null' ? undefined : res.headers['content-type'];
            return new File([res.data], fileName, {type: contentType});
        });

I've now also added code to support geojson. However, I noticed that the readGeoJson method returns an object includes an errors list. Currently, my code does not perform any actions based on the presence of errors in this list, such as throwing a new Error. Implementing such behavior would lead to the failure of existing tests, as the caput-mundi.geojson file in test resources include the crs attribute. This triggers the error 'old-style crs member is not recommended, this object is equivalent to the default and should be removed' when the geojsonhint method is called within readGeoJson.

I would greatly appreciate any further advice you could provide. Thank you.

FixBug .geojson file not supported in macos.
- added codes to support geojson in processFiles.jsx
- fixed bug in testData.js in order to give a type by initialing a new File instance

On behalf of DB Systel GmbH
@tdipisa tdipisa linked an issue Apr 9, 2024 that may be closed by this pull request
@tdipisa tdipisa modified the milestones: 2024.01.01, 2024.01.02, 2024.02.00 May 28, 2024
@tdipisa tdipisa requested a review from offtherailz October 2, 2024 09:28
@tdipisa tdipisa modified the milestones: 2024.02.00, 2025.01.00, 2024.02.01 Oct 2, 2024
@tdipisa tdipisa added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Oct 8, 2024
@tdipisa tdipisa requested review from dsuren1 and removed request for offtherailz October 14, 2024 14:13
@dsuren1 dsuren1 enabled auto-merge (squash) October 21, 2024 08:20
@tdipisa tdipisa added the bug label Oct 21, 2024
@tdipisa tdipisa closed this Oct 21, 2024
auto-merge was automatically disabled October 21, 2024 08:31

Pull request was closed

@tdipisa tdipisa reopened this Oct 21, 2024
@dsuren1 dsuren1 enabled auto-merge (squash) October 21, 2024 08:34
@dsuren1 dsuren1 merged commit afafc5c into geosolutions-it:master Oct 21, 2024
5 checks passed
@dsuren1
Copy link
Contributor

dsuren1 commented Oct 21, 2024

@tdipisa @offtherailz Since this scenario can be tested effectively only on mac. Can I test and proceed with the backport?

@tdipisa
Copy link
Member

tdipisa commented Oct 21, 2024

You can test with MAC but let's also wait for functional tests as usual made by @ElenaGallo. No? Did I miss something?

@dsuren1
Copy link
Contributor

dsuren1 commented Oct 22, 2024

@tdipisa @ElenaGallo Works fine in DEV and tested on a Mac with geosjon file

dsuren1 pushed a commit to dsuren1/MapStore2 that referenced this pull request Oct 22, 2024
* geosolutions-it#10081
FixBug .geojson file not supported in macos.
- In macOS .geojson file will be identified as type of "application/geo+json". So in checkFileType and readFile functions, they will not go into MIME_LOOKUPS list to find the corresponding type.
- Check first in MIME_LOOKUPS list and then the file.type will solve this problem.
On behalf of DB Systel GmbH

* geosolutions-it#10082
FixBug .geojson file not supported in macos.
- added codes to support geojson in processFiles.jsx
- fixed bug in testData.js in order to give a type by initialing a new File instance

On behalf of DB Systel GmbH

(cherry picked from commit afafc5c)
@ElenaGallo
Copy link
Contributor

testpassed, @dsuren1 please backport to 2024.02.xx. Thanks

tdipisa pushed a commit that referenced this pull request Oct 24, 2024
* #10081
FixBug .geojson file not supported in macos.
- In macOS .geojson file will be identified as type of "application/geo+json". So in checkFileType and readFile functions, they will not go into MIME_LOOKUPS list to find the corresponding type.
- Check first in MIME_LOOKUPS list and then the file.type will solve this problem.
On behalf of DB Systel GmbH

* #10082
FixBug .geojson file not supported in macos.
- added codes to support geojson in processFiles.jsx
- fixed bug in testData.js in order to give a type by initialing a new File instance

On behalf of DB Systel GmbH

(cherry picked from commit afafc5c)

Co-authored-by: congchen1101 <161452326+congchen1101@users.noreply.github.com>
@tdipisa tdipisa removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Oct 24, 2024
rmelarab-ngs pushed a commit to ngsllc/MapStore2 that referenced this pull request Nov 5, 2024
* geosolutions-it#10081
FixBug .geojson file not supported in macos.
- In macOS .geojson file will be identified as type of "application/geo+json". So in checkFileType and readFile functions, they will not go into MIME_LOOKUPS list to find the corresponding type.
- Check first in MIME_LOOKUPS list and then the file.type will solve this problem.
On behalf of DB Systel GmbH

* geosolutions-it#10082
FixBug .geojson file not supported in macos.
- added codes to support geojson in processFiles.jsx
- fixed bug in testData.js in order to give a type by initialing a new File instance

On behalf of DB Systel GmbH
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.geojson file not supported with mac os
5 participants